-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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.
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
Keep resolved defs in path prefixes and emit them in save-analysis #54145
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
r? @petrochenkov cc @alexcrichton @nikomatsakis Could you add support in stability checking, for looking at these |
src/libsyntax/parse/parser.rs
Outdated
} else { | ||
// Generic arguments are not found. | ||
PathSegment::from_ident(ident) | ||
PathSegment::from_ident(ident,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: stray comma
// }) | ||
// .collect(), | ||
// span: path.span, | ||
// }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
CI is failing + this needs a perf run (new fields in path segments + a lot more node IDs). |
src/librustc_resolve/lib.rs
Outdated
.collect::<Vec<_>>(); | ||
self.smart_resolve_path_fragment(id, qself, segments, path.span, source, crate_lint) | ||
} | ||
|
||
fn smart_resolve_path_fragment(&mut self, | ||
id: NodeId, | ||
qself: Option<&QSelf>, | ||
path: &[Ident], | ||
path: &[(Ident, Option<NodeId>)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve code is a lot uglier now due to these &[(Ident, Option<NodeId>)]
things, but hopefully this can be refactored later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a resolve-specific PathSegment
struct would be better?
Long-term we might instead want nested |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I've addressed reviewer comments, but looks like we're still failing CI. I'm away for a few days and will investigate when I get back |
☔ The latest upstream changes (presumably #54086) made this pull request unmergeable. Please resolve the merge conflicts. |
721229b
to
404c55c
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #54389) made this pull request unmergeable. Please resolve the merge conflicts. |
src/librustc/hir/map/mod.rs
Outdated
@@ -1093,6 +1095,7 @@ impl<'a> print::State<'a> { | |||
Node::AnonConst(a) => self.print_anon_const(&a), | |||
Node::Expr(a) => self.print_expr(&a), | |||
Node::Stmt(a) => self.print_stmt(&a), | |||
Node::PathSegment(_) => bug!("cannot print PathSegment"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably implement this, to avoid cascading issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -337,6 +337,8 @@ impl fmt::Display for Path { | |||
pub struct PathSegment { | |||
/// The identifier portion of this path segment. | |||
pub ident: Ident, | |||
pub id: Option<NodeId>, | |||
pub def: Option<Def>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason these are Option
? I'd say fix and/or document them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use them for synthesized path segments. I've added docs.
Still needs a rebase, ICE fix, and @rust-timer run. |
Insufficient permissions to issue commands to rust-timer. |
404c55c
to
81ee74b
Compare
Rebased and ICE fixed. How do I do a rust-timer run? I collected data locally but couldn't run the site. |
📌 Commit 6dd5bb1 has been approved by |
Keep resolved defs in path prefixes and emit them in save-analysis Closes rust-dev-tools/rls-analysis#109 r? @eddyb or @petrochenkov
☀️ Test successful - status-appveyor, status-travis |
Tested on commit rust-lang/rust@7b0735a. Direct link to PR: <rust-lang/rust#54145> 💔 rls on windows: test-pass → test-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → test-fail (cc @nrc, @rust-lang/infra).
The RLS test failures:
|
save-analysis: bug fix and optimisation. The first commit fixes a bug in name resolution and save-analysis (introduced in #54145) and removes an unused parameter. This fixes the RLS tests, which are currently blocking distribution of the RLS. The second commit removes macro uses from save-analysis data, since these are never used, they just take up space. r? @petrochenkov
save-analysis: bug fix and optimisation. The first commit fixes a bug in name resolution and save-analysis (introduced in #54145) and removes an unused parameter. This fixes the RLS tests, which are currently blocking distribution of the RLS. The second commit removes macro uses from save-analysis data, since these are never used, they just take up space. r? @petrochenkov
Give each PathSegment a NodeId Store a resolved def on hir::PathSegment save-analysis: remove hacky, unnecessary code now that we have spans for every ident dump data for prefix path segments dump refs for path segments in save-analysis Requires adding path segments to the hir map Fix tests and rustdoc save-analysis: handle missing field names FIxes rust-lang/rls#1031 rebasing and reviewer changes Primarily refactoring `(Ident, Option<NodeId>)` to `Segment` Fix tests and assertions; add some comments more reviewer changes
beta backport rollup Backports of some beta-approved PRs - [x] #55385: NLL: cast causes failure to promote to static - [x] #56043: remove "approx env bounds" if we already know from trait - [x] #56003: do not propagate inferred bounds on trait objects if they involve `Self` - [x] #55852: Rewrite `...` as `..=` as a `MachineApplicable` 2018 idiom lint - [x] #55804: rustdoc: don't inline `pub use some_crate` unless directly asked to - [x] #56059: Increase `Duration` approximate equal threshold to 1us - [x] Keep resolved defs in path prefixes and emit them in save-analysis #54145 - [x] Adjust Ids of path segments in visibility modifiers #55487 - [x] save-analysis: bug fix and optimisation. #55521 - [x] save-analysis: be even more aggressive about ignorning macro-generated defs #55936 - [x] save-analysis: fallback to using path id #56060 - [x] save-analysis: Don't panic for macro-generated use globs #55879 - [x] Add temporary renames to manifests for rustfmt/clippy #56081 - [x] Revert #51601 #56049 - [x] Fix stability hole with `static _` #55983 - [x] #56077 - [x] Fix Rustdoc ICE when checking blanket impls #55258 - [x] Updated RELEASES.md for 1.31.0 #55678 - [x] ~~#56061~~ #56111 - [x] Stabilize `extern_crate_item_prelude` #56032 Still running tests locally, and I plan to backport @nrc's other PRs too (cc @petrochenkov -- thanks for the advice)
Closes rust-dev-tools/rls-analysis#109
r? @eddyb or @petrochenkov