-
Notifications
You must be signed in to change notification settings - Fork 13k
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
save_analysis: work on HIR tree instead of AST #72882
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
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 |
8817921
to
8a69ec2
Compare
It appears you left some dead code commented out. Did you forget to remove it? |
Well originally, i thought tests would fail and that it would take me a while to fix those so i created a draft PR; knowing there would be some cleanups to do. But then tests passed and I converted into a real PR and forgot to remove the dead code. |
8a69ec2
to
5b014fb
Compare
I ran the
To debug i dumped the analysis using
The only difference is that some id's seem wrong with the current nightly and "fixed' with this change but that does not really explain why the tests are failing. If we take a closer look at
Whereas the test expect to find range (no found):
and (found):
I am not sure how to go forward with this or even if the failures are not in fact just fixes? |
That's great, thanks so much for this! I'll gladly review that tomorrow morning since it's getting late here 😴 |
@Xanewok i've updated the pasted links because https://paste.ee seems unavailable right now |
8d877d9
to
7da8c09
Compare
@Xanewok anything else i should take care of? |
Thank you for your work and for your patience! I'd say this looks good to go :) Since this has a potential to cause some issues (it's a considerable rewrite!), I'd sleep better if we waited for the beta cut (happening around now) and then land this. Does that sound good? |
@Xanewok beta has landed i believe. Once this is merged it will break RLS from compiling because the prototype of |
Yep, that’s the one!
Since you’re testing the RLS already and have a fix, could you submit a PR
there and we coordinate the push together?
I can push the relevant RLS update to a separate branch, we’ll include the
submodule update in this PR to that branch and after that I’ll merge the
RLS branch to its master.
This way we can avoid the breakage altogether 👍
…On Wed, 3 Jun 2020 at 10:45, Élie ROUDNINSKI ***@***.***> wrote:
@Xanewok <https://github.com/Xanewok> beta has landed i believe. Once
this is merged it will break RLS from compiling because the prototype of
process_crate changed. Is https://github.com/rust-lang/rls the right repo
to provide a fix?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#72882 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXTFXKOBU3R5PP4I4GHMH3RUYES7ANCNFSM4NPYCF3A>
.
|
@Xanewok here is the PR for |
Merged into a separate branch: rust-lang/rls@8d7a716 Could you include the submodule bump in this PR? All you need to do is to: |
@Xanewok done 👍 |
Awesome, thank you! @bors r+ |
📌 Commit 077658d6578a3d3919b2e9d3000138d13a02eecf has been approved by |
Let's bump the prio since this updates the submodule as well: @bors p=1 |
There are still a few FIXME in the code. Do you plan to address them before merging? |
r? @Xanewok |
⌛ Testing commit d524ba97e398ba0513bffd55f8cfdb4b87ec7c57 with merge 52875e00e8d38b5a995536c868a885ef506b71f7... |
💔 Test failed - checks-azure |
d524ba9
to
70228f9
Compare
@Xanewok failure looks spurious. Just rebased for the sake of it. |
📌 Commit 70228f9 has been approved by |
☀️ Test successful - checks-azure |
// Drop AST after creating GlobalCtxt to free memory | ||
let _timer = sess.prof.generic_activity("drop_ast"); | ||
mem::drop(queries.expansion()?.take()); |
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.
This needs a block around it, the _timer
is supposed to be dropped after the next statement, in order for drop_ast
to track only that, and not the rest of this large block. (it's not a big deal, but I was baffled as to why drop_ast
had so much variance, or any children at all, in the profiling data)
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.
Sorry about that! Should be fixed by #75575
In order to reduce the uses of
NodeId
s in the compiler,save_analysis
crate has been reworked to operate on the HIR tree instead of the AST.cc #50928