-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
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
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.
@@ -63,7 +63,7 @@ none() | |||
{{#classes}} | |||
{{#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... 🤷
@@ -363,10 +361,10 @@ impl<'a> Translator<'a> { | |||
.as_ref() | |||
.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
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.
No description provided.