-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rust: upgrade rust-analyzer to 0.0.288
#19524
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
Conversation
cc2e8ab to
28b504a
Compare
rust-analyzer to 0.0.279rust-analyzer to 0.0.281
geoffw0
left a comment
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.
QL changes LGTM.
I've raised some questions on the DCA run, it looks like a net gain but there are some regressions in there that deserve discussing.
Extractor changes not reviewed.
rust/ql/test/library-tests/type-inference/type-inference.expected
Outdated
Show resolved
Hide resolved
rust-analyzer to 0.0.281rust-analyzer to 0.0.285
|
Looks good to me. The additional path resolution inconsistencies are a bit annoying though. Any idea where they come from? |
|
Judging by the second DCA run it looks like we're extracting significantly fewer files successfully as well - though the run did fail so I don't want to read too much into that. |
|
Don't know, there are still loads of unexpanded macros around :sigh: |
|
I've opened rust-lang/rust-analyzer#19931 (as I couldn't reopen rust-lang/rust-analyzer#19873). |
rust-analyzer to 0.0.285rust-analyzer to 0.0.287
|
oh well, there's a weird path resolution test error I'll need to look into on Monday :sigh: |
rust-analyzer to 0.0.287rust-analyzer to 0.0.288
5cf6b5c to
7edae1e
Compare
aibaars
left a comment
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.
Looks good to me.
| {{#final}} | ||
| or | ||
| result = getImmediateChildOf{{name}}(e, index, partialAccessor) | ||
| index = min(int i | result = getImmediateChildOf{{name}}(e, i, partialAccessor) | i) |
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.
Let's revert this change.
| if err.span().anchor.file_id == semantics.hir_file_for(node.syntax()) { | ||
| let hir_file_id = semantics.hir_file_for(node.syntax()); | ||
| if Some(err.span().anchor.file_id.file_id()) | ||
| == hir_file_id.file_id().map(|f| f.file_id(semantics.db)) |
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.
how did you figure out that you needed the file_id of the file_id of the file_id of file_id ? ;-)
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.
turtles all the way down... 🤷
| .and_then(|s| s.expand_macro_call(mcall)) | ||
| { | ||
| self.emit_macro_expansion_parse_errors(mcall, &expanded); | ||
| self.emit_macro_expansion_parse_errors(mcall, &expanded.value); |
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.
Does expanded also have an err field? If so, let log it, hopefully it'll provide some useful information of why an expansion failed?
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.
yes, the latest version of the PR does that
rust/ql/test/library-tests/dataflow/sources/InlineFlow.expected
Outdated
Show resolved
Hide resolved
|
Don't forgot to commit empty expected files for |
190ab52 to
1823a22
Compare
1823a22 to
13b28e2
Compare
| | macro_expansion.rs:63:9:63:32 | include_str!... | | ||
| warnings | ||
| | included/included.rs:1:1:1:1 | semantic analyzer unavailable (not loaded as its own module, probably included by `!include`) | | ||
| | macro_expansion.rs:56:9:56:31 | macro expansion failed: could not resolve macro 'concat' | |
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.
Does these changes mean we expand fewer macros?
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.
Yes, it does unfortunately. It is likely that the cause is the same as for rust-lang/rust-analyzer#20037 . See also the DCA report for more insight in the impact.
|
Database build time, average 8 % speedup 🎉 |
No description provided.