Initial version. by ltratt · Pull Request #1 · softdevteam/depub

ltratt · 2021-03-26T19:37:54Z

Please see README.md for details.

ltratt · 2021-03-26T19:38:00Z

bors try

ltratt · 2021-03-26T19:39:44Z

bors try-

ltratt · 2021-03-26T19:39:48Z

bors try

ltratt · 2021-03-26T19:41:52Z

bors try-

ltratt · 2021-03-26T23:15:42Z

bors try

bors · 2021-03-26T23:17:26Z

try

Build succeeded:

buildbot/buildbot-build-script

ltratt · 2021-03-28T18:03:09Z

@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.

bjorn3 · 2021-03-28T19:28:11Z

~~If I understand correctly what this project does I think #![deny(unreachable_pub)] + cargo fix covers the full functionality.~~

https://github.com/est31/warnalyzer/ is closer to what this does.

ltratt · 2021-03-28T19:57:51Z

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 save-analysis or similar.

[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.]

bjorn3 · 2021-03-28T20:01:31Z

Warnalyzer is for multi crate usage. It was used for rust-lang/rust#83185.

ltratt · 2021-03-28T20:03:17Z

Ah, yes, I see I misread the README! Once the relevant PRs have gone into yk, I might try running warnalyzer over that repo. AFAICS it doesn't do anything about visibility (which is depub's raison d'être, even though it goes about it in a rather crude manner).

vext01 · 2021-03-29T09:00:11Z

Cargo.toml

+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


I usually nuke this line.

Fixed in f093ea5.

vext01 · 2021-03-29T09:03:45Z

README.md

+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!
+


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".

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?

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.

Fixed (hopefully) in fc38494.

vext01 · 2021-03-29T09:06:58Z

README.md

+and execute:
+
+```
+$ find . -name "*.rs" | \


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?

Way too much complexity for now! Also, at least at the moment, it's sometimes useful to only pass a subset of files.

vext01 · 2021-03-29T09:11:00Z

src/main.rs

+                "crate" => PubKind::Crate,
+                "super" => PubKind::Super,
+                _ => {
+                    // FIXME: this captures things we don't need to deal with (e.g. `pub(self)`),


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?

Probably one should plug into rustc. That's no small job!

Yeah, that's why rust-analyzer is probably more attractive. But anyway, we won't do that right now.

vext01 · 2021-03-29T09:19:15Z

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.

vext01 · 2021-03-29T09:20:13Z

README.md

+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


even though that's not what you want

I think?

Fixed in 7f98833.

vext01 · 2021-03-29T09:22:17Z

I think this is ready. Please squash.

ltratt · 2021-03-29T09:22:57Z

Squashed.

vext01 · 2021-03-29T09:24:13Z

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+

vext01 · 2021-03-29T09:25:18Z

after bors send its first status notification, you can add bors too.

done

bors · 2021-03-29T09:26:17Z

Build succeeded:

buildbot/buildbot-build-script

ltratt assigned vext01 Mar 26, 2021

bors bot added a commit that referenced this pull request Mar 26, 2021

Try #1:

b1003ec

bors bot added a commit that referenced this pull request Mar 26, 2021

Try #1:

3a9d81e

bors bot added a commit that referenced this pull request Mar 26, 2021

Try #1:

dd1002d

ltratt mentioned this pull request Mar 28, 2021

Depub ykjit/yk#309

Merged

vext01 reviewed Mar 29, 2021

View reviewed changes

Initial version.

3040199

ltratt force-pushed the initial_version branch from 7f98833 to 3040199 Compare March 29, 2021 09:22

bors bot merged commit ee06521 into softdevteam:master Mar 29, 2021

ltratt deleted the initial_version branch March 29, 2021 09:37

Conversation

ltratt commented Mar 26, 2021

Uh oh!

ltratt commented Mar 26, 2021

Uh oh!

ltratt commented Mar 26, 2021

Uh oh!

ltratt commented Mar 26, 2021

Uh oh!

ltratt commented Mar 26, 2021

Uh oh!

ltratt commented Mar 26, 2021

Uh oh!

bors bot commented Mar 26, 2021

try

Uh oh!

ltratt commented Mar 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn3 commented Mar 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltratt commented Mar 28, 2021

Uh oh!

bjorn3 commented Mar 28, 2021

Uh oh!

ltratt commented Mar 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 commented Mar 29, 2021

Uh oh!

vext01 Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 commented Mar 29, 2021

Uh oh!

ltratt commented Mar 29, 2021

Uh oh!

vext01 commented Mar 29, 2021

Uh oh!

vext01 commented Mar 29, 2021

Uh oh!

bors bot commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ltratt commented Mar 28, 2021 •

edited

Loading

bjorn3 commented Mar 28, 2021 •

edited

Loading

vext01 Mar 29, 2021 •

edited

Loading