Skip to content

Allow filtering analysis by reachability #46011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Nov 23, 2017
Merged

Allow filtering analysis by reachability #46011

merged 4 commits into from
Nov 23, 2017

Conversation

euclio
Copy link
Contributor

@euclio euclio commented Nov 15, 2017 •
edited
Loading

Fixes #43521.
Fixes rust-dev-tools/rls-analysis#79.

This PR allows a user to filter items present in the save-analysis data by setting the reachable_only config option. This option is intended for use by the new rustdoc. The PR isn't quite finished, because it's dependent on a new release of rls-data, but I want to make sure that the approach is valid.

rust-dev-tools/rls-analysis#79 mentions that pub use might need to be handled, but my thinking is that the consumer of the analysis data would be able to infer which imports are pub use, and which items are only reachable through pub use, so that doesn't need to be handled here.

r? @nrc

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2017
nrc
nrc reviewed Nov 16, 2017
View reviewed changes
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This is easier than I expected - is_reachable seems very convenient!

What exactly do you need from rls-data? Do we just need to publish a new version from master or is there code that needs to land there too?

src/librustc_save_analysis/dump_visitor.rs Outdated
&Access {
public: item.vis == ast::Visibility::Public,
reachable: self.save_ctxt.analysis.access_levels.is_reachable(item.id),
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks kinda boilerplatey - could you DRY it out by using a function or macro?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This code" = initialising Access

@euclio
Copy link
Contributor Author

euclio commented Nov 16, 2017 •
edited
Loading

For rls_data, I just need the latest code on master, which has the reachable_only config option. Although, now that I look at the source for AccessLevels I'm wondering if we actually want to use exported instead of reachable for rustdoc? (Maybe not, since private-in-public is a hard error).

As for removing boilerplate, it would be easy enough to create it from the AccessLevels struct in a helper. I can address that.

@nrc
Copy link
Member

nrc commented Nov 20, 2017

I've published rls-data 0.13 which has the change you need.

@nrc
Copy link
Member

nrc commented Nov 20, 2017

r+ with the rls-data dep change

@euclio euclio force-pushed the reachability-redux branch from aadec89 to b082f78 Compare November 20, 2017 04:32
@euclio euclio changed the title [WIP] Allow filtering analysis by reachability Allow filtering analysis by reachability Nov 20, 2017
@euclio
Copy link
Contributor Author

euclio commented Nov 20, 2017

This should be ready to go.

@nrc
Copy link
Member

nrc commented Nov 21, 2017

@bors: r+

@bors
Copy link
Collaborator

bors commented Nov 21, 2017

📌 Commit b082f78 has been approved by nrc

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2017
@bors
Copy link
Collaborator

bors commented Nov 22, 2017

⌛ Testing commit b082f78 with merge 23132d8c7ea9f986f6f0fb94cfc474bce3853baa...

@bors
Copy link
Collaborator

bors commented Nov 22, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Nov 22, 2017

Failure from multiple dist builds when building stage1-std.

[00:23:57] error: internal compiler error: unexpected panic
[00:23:57] 
[00:23:57] note: the compiler unexpectedly panicked. this is a bug.
[00:23:57] 
[00:23:57] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:23:57] 
[00:23:57] note: rustc 1.23.0-nightly (23132d8c7 2017-11-22) running on x86_64-unknown-linux-gnu
[00:23:57] 
[00:23:57] thread 'rustc' panicked at 'Could not deserialize save-analysis config: MissingFieldError("reachable_only")', /checkout/src/libcore/result.rs:906:4
[00:23:57] note: Run with `RUST_BACKTRACE=1` for a backtrace.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 22, 2017
@euclio
Copy link
Contributor Author

euclio commented Nov 22, 2017

Sorry, I'm unfamiliar with how the dist builders work. How can I reproduce this locally?

@kennytm
Copy link
Member

kennytm commented Nov 22, 2017

@euclio You could run using ./x.py dist. Since the error is about the config, perhaps you need to update the code involving the RUST_SAVE_ANALYSIS_CONFIG environment variable in bootstrap...

@euclio
Copy link
Contributor Author

euclio commented Nov 22, 2017

Hm, I'm not able to reproduce the failure locally but I've updated the likely cause.

@kennytm
Copy link
Member

kennytm commented Nov 22, 2017

@bors r=nrc

@bors
Copy link
Collaborator

bors commented Nov 22, 2017

📌 Commit 794ada0 has been approved by nrc

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 22, 2017
@bors
Copy link
Collaborator

bors commented Nov 23, 2017

⌛ Testing commit 794ada0 with merge 10ef344...

bors added a commit that referenced this pull request Nov 23, 2017
@bors
Auto merge of #46011 - euclio:reachability-redux, r=nrc
10ef344 
Allow filtering analysis by reachability

Fixes #43521.
Fixes rust-dev-tools/rls-analysis#79.

This PR allows a user to filter items present in the save-analysis data by setting the `reachable_only` config option. This option is intended for use by the new rustdoc. The PR isn't quite finished, because it's dependent on a new release of rls-data, but I want to make sure that the approach is valid.

rust-dev-tools/rls-analysis#79 mentions that `pub use` might need to be handled, but my thinking is that the consumer of the analysis data would be able to infer which imports are `pub use`, and which items are only reachable through `pub use`, so that doesn't need to be handled here.

r? @nrc
@bors
Copy link
Collaborator

bors commented Nov 23, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 10ef344 to master...

@bors bors merged commit 794ada0 into rust-lang:master Nov 23, 2017
@bors bors mentioned this pull request Nov 23, 2017
Merged
@euclio euclio mentioned this pull request Nov 24, 2017
Closed
@martinlindhe martinlindhe mentioned this pull request Nov 28, 2017
Closed
@euclio euclio deleted the reachability-redux branch February 7, 2019 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrc nrc nrc left review comments

Assignees

@nrc nrc

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@euclio @nrc @bors @kennytm