Initial version. #1
Conversation
|
bors try |
|
bors try- |
|
bors try |
|
bors try- |
|
bors try |
tryBuild succeeded: |
|
@vext01 This is now ready for review (I suggest reviewing this in one go; the individual commits are no longer very useful and will ultimately need squashing down into one). See ykjit/yk#309 for an example of depub in use. |
|
https://github.com/est31/warnalyzer/ is closer to what this does. |
|
https://github.com/est31/warnalyzer/ looks interesting, although it seems both more clever (it uses rustc internals) and less clever (single crate only) than depub. In an ideal world, someone might take depub's idea and make a more rigorous tool out of it, perhaps using [FWIW, I wouldn't be surprised if there is some dead code left in yk -- ykpack is difficult to process accurately -- although hopefully not much.] |
|
Warnalyzer is for multi crate usage. It was used for rust-lang/rust#83185. |
|
Ah, yes, I see I misread the README! Once the relevant PRs have gone into yk, I might try running |
Cargo.toml
Outdated
| readme = "README.md" | ||
| license = "Apache-2.0 OR MIT" | ||
|
|
||
| # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html |
| replaces with the original and tries the next item. Note that `depub` is | ||
| inherently destructive: it overwrites files as it operates, so do not run it on | ||
| source code that you do not want altered! | ||
|
|
There was a problem hiding this comment.
Do we need some kind of warning stating that: in general you cannot know what pub stuff downstream crates may consume, so you should only really use this tool in "enclosed ecosystems".
There was a problem hiding this comment.
in general you cannot know what pub stuff downstream crates may consume, so you should only really use this tool in "enclosed ecosystems".
I thought that was sort-of obvious in this bit:
`depub` minimises the visibility of such items in files passed to it, using a
user-specified command (e.g. `cargo check`) as an oracle to tell if its
reduction of an item's visibility is valid or not.
but maybe it isn't?
There was a problem hiding this comment.
Well, I knew what you meant, but since the implications of getting it wrong could be quite bad, I don't think it hurts to make it explicit.
There was a problem hiding this comment.
Fixed (hopefully) in fc38494.
| and execute: | ||
|
|
||
| ``` | ||
| $ find . -name "*.rs" | \ |
There was a problem hiding this comment.
This is begging for a cargo plugin to hide the file search from users. Just as cargo fmt finds and formats all of the rust files in a project, perhapscargo depub could find and depub all of the rust files in a project. Maybe later?
There was a problem hiding this comment.
Way too much complexity for now! Also, at least at the moment, it's sometimes useful to only pass a subset of files.
| "crate" => PubKind::Crate, | ||
| "super" => PubKind::Super, | ||
| _ => { | ||
| // FIXME: this captures things we don't need to deal with (e.g. `pub(self)`), |
There was a problem hiding this comment.
A future version of this tool could use a parser (or maybe rust-analyzer) so as to only mutate things that really are visibility modifiers?
There was a problem hiding this comment.
Probably one should plug into rustc. That's no small job!
There was a problem hiding this comment.
Yeah, that's why rust-analyzer is probably more attractive. But anyway, we won't do that right now.
|
About warnalyzer: I think depub (whilst dumber) is more flexible, as it lets you provide an arbitrarily complex oracle command that could span multiple workspaces if necessary. E.g. Using depub the oracle could include (in addition to the normal yk test suite) a build of ykrustc to check for junk in ykpack. |
README.md
Outdated
| reduction of an item's visibility is valid or not. Note that `depub` is | ||
| entirely guided by the oracle command: if the code it compiles happens not to | ||
| use part of an intentionally public interface, then `depub` is likely to | ||
| suggest reducing its visibility even though that's what you want. The broader |
There was a problem hiding this comment.
even though that's not what you want
I think?
|
I think this is ready. Please squash. |
|
Squashed. |
|
OK, let's see if bors works. I've turned on the required check for buildbot, and after bors send its first status notification, you can add bors too. bors r+ |
done |
|
Build succeeded: |
Please see
README.mdfor details.