Refactor fingerprint reconstruction #89343
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @rust-lang/wg-incr-comp -- I'm not sure if folks recall the intent of has_params, which might be helpful to determine if this is the "right" thing to do. |
|
IIUC, the crash happens when trying to debug-print a DepNode whch parameter () by mistakenly trying to extract a DefId from a Fingerprint which is not a proper DefPathHash. The bug is therefore in |
|
Hm, that seems like a good thing to tackle, I agree. I think this PR is still desirable (or we should just remove has_params), since it seems like the meaning of that is not too clear right now. I'll have to dig into the 2 manually declared queries with has_params: false, and see if that's significant for them... |
cjgillot
added
S-waiting-on-author
Mark-Simulacrum
force-pushed
the
no-args-queries
branch
from
b2b53bb to
0d1ff33
Compare
Mark-Simulacrum
changed the title
Mark-Simulacrum
force-pushed
the
no-args-queries
branch
2 times, most recently
from
175631d to
3d73c4d
Compare
Mark-Simulacrum
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
force-pushed
the
no-args-queries
branch
2 times, most recently
from
9173a20 to
63aaf88
Compare
|
Alright, replaced pretty much the whole PR with different patch -- and extracted #89466 for the base macro changes which are not actually needed for this PR now. I looked into fully removing has_params but didn't end up making that change. The only uses left are the debug assert in DepKind::new_no_params and the debug_node method. The latter could be removed with no loss, I believe -- just a difference in how nodes look (printed as |
| match kind.fingerprint_style() { | ||
| FingerprintStyle::Opaque => Err(()), | ||
| FingerprintStyle::Unit => { | ||
| if !kind.has_params { |
There was a problem hiding this comment.
FingerprintStyle::Unit should now be equivalent to has_params == false, isn't it? We should be able to get rid of has_params.
There was a problem hiding this comment.
Not currently. A query with () key currently "has_params"; the only two things to not have params are Null and TraitSelect which are not generated through the normal macro infrastructure. I dropped the "fix" for that from this branch for now, though I can re-add it, it's not big (b2b53bb9a4f2f5e1f96f8f01ad7b378f5eb1b4d1).
|
@bors r+ |
|
📌 Commit 63aaf88 has been approved by |
bors
added
S-waiting-on-bors
…cjgillot
Refactor fingerprint reconstruction
This PR replaces can_reconstruct_query_key with fingerprint_style, which returns the style of the fingerprint for that query. This allows us to avoid trying to extract a DefId (or equivalent) from keys which *are* reconstructible because they're () but not as DefIds.
This is done with the goal of fixing -Zdump-dep-graph, which seems to have broken a while ago (I didn't try to bisect). Currently even on a `fn main() {}` file it'll ICE (you need to also pass -Zquery-dep-graph for it to work at all), and this patch indirectly fixes the cause of that ICE. This also adds a test for it continuing to work.
|
☔ The latest upstream changes (presumably #89266) made this pull request unmergeable. Please resolve the merge conflicts. |
Keys can be reconstructed from fingerprints that are not DefPathHash, but then we cannot extract a DefId from them.
Mark-Simulacrum
force-pushed
the
no-args-queries
branch
from
63aaf88 to
85c4fd8
Compare
|
@bors r=cjgillot rollup=iffy |
|
📌 Commit 85c4fd81799de01e8eb3a84f96a13baafc8de407 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 85c4fd81799de01e8eb3a84f96a13baafc8de407 with merge 8f2565b630f71928671276797f4659fcc679d795... |
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
Mark-Simulacrum
force-pushed
the
no-args-queries
branch
from
85c4fd8 to
415a9a2
Compare
|
@bors r=cjgillot rollup=iffy |
|
📌 Commit 415a9a2 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (15491d7): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Debug logging during incremental compilation had been broken for some time, until rust-lang#89343 fixed it (among other things). Add a test so this is less likely to break without being noticed. This test is nearly a copy of the `src/test/ui/rustc-rust-log.rs` test, but tests debug logging in the incremental compliation code paths.
…t, r=Mark-Simulacrum Add test for debug logging during incremental compilation Debug logging during incremental compilation had been broken for some time, until rust-lang#89343 fixed it (among other things). Add a test so this is less likely to break without being noticed. This test is nearly a copy of the `src/test/ui/rustc-rust-log.rs` test, but tests debug logging in the incremental compliation code paths.
…t, r=Mark-Simulacrum Add test for debug logging during incremental compilation Debug logging during incremental compilation had been broken for some time, until rust-lang#89343 fixed it (among other things). Add a test so this is less likely to break without being noticed. This test is nearly a copy of the `src/test/ui/rustc-rust-log.rs` test, but tests debug logging in the incremental compliation code paths.
This PR replaces can_reconstruct_query_key with fingerprint_style, which returns the style of the fingerprint for that query. This allows us to avoid trying to extract a DefId (or equivalent) from keys which are reconstructible because they're () but not as DefIds.
This is done with the goal of fixing -Zdump-dep-graph, which seems to have broken a while ago (I didn't try to bisect). Currently even on a
fn main() {}file it'll ICE (you need to also pass -Zquery-dep-graph for it to work at all), and this patch indirectly fixes the cause of that ICE. This also adds a test for it continuing to work.