Change hashing/comparison of LintExpectationId (fix ICE)

[Bryntet]

View all comments

this fixes #154878 by removing attr_id from LintExpectationId and instead using target_span + attr_index + lint_index in combination to refer to a unique lint expectation

[rustbot]

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 10 candidates

[Bryntet]

I couldn't figure out how to make a regression test for this due to it needing the incremental cache, if you know how to I'd love to add it

[JonathanBrouwer]

Adding a regression test should be possible as an incremental test

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Bryntet]

@rustbot ready

added regression test and tested that it indeed does fail on main

[matthiaskrgr]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 4720e09 (4720e09e56a6568f683319e01bd582d8ae6ca6b8, parent: a92a99ef1479a9987b0ef76a064c2bd8e779babd)

[rust-timer]

Finished benchmarking commit (4720e09): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[2.5%, 2.5%]	1

Cycles

Results (secondary 5.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.0%	[5.0%, 5.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 487.739s -> 488.081s (0.07%)
Artifact size: 395.17 MiB -> 395.14 MiB (-0.01%)

[Urgau]

Any idea why removing AttrId is necessary?

[JonathanBrouwer]

Any idea why removing AttrId is necessary?

AttrIds are part of the HIR and can therefore be serialized and deserialized.
The problems are that:

• AttrIds are not stable: When restarting the compiler it starts counting from 0 again, we can't simply store the AttrId in the incremental files

I'm a bit confused why the old code never had this problem, because AttrId is also part of hir::AttrItem so will also be encoded/decoded there. Any idea @Bryntet ?

[Bryntet]

Any idea why removing AttrId is necessary?

AttrIds are part of the HIR and can therefore be serialized and deserialized. The problems are that:

* `AttrId`s are not stable: When restarting the compiler it starts counting from 0 again, we can't simply store the `AttrId` in the incremental files

I'm a bit confused why the old code never had this problem, because AttrId is also part of hir::AttrItem so will also be encoded/decoded there. Any idea @Bryntet ?

Previously, AttrId wasn't stored on LintExpectationId::Stable, instead, it used the hir_id + attr_index, to query all attrs on the hir_id, and then take the attr that was there, it would then call hir::Attribute::id, but this of course leads to a panic on any hir::Attribute::Parsed, which I converted the lint attrs into being

So therefore I instead decided to store the AttrId inside LintExpectationId::Stable, seemed like a logical solution at the time.

But this as we now see led to this bug

@JonathanBrouwer

[Bryntet]

here's the old code that got AttrId on the stable variant

[Bryntet]

Also, since this no longer requires AttrId, does that mean we could remove eval_always from the check_expectations query?

Edit: if we were to do this, that should probably be it's own, separate PR though.

[JonathanBrouwer]

I discovered this issue while trying to figure out if using target_span is correct: #155008
Would like to see if we can also address this in this PR, since I think switching to using spans might make this issue harder to solve

[cjgillot]

I agree this should not use spans. Spans are for diagnostics and debuginfo. Good path compilation should not use them.

[Bryntet]

I discovered this issue while trying to figure out if using target_span is correct: #155008 Would like to see if we can also address this in this PR, since I think switching to using spans might make this issue harder to solve

#155038 fixes #155008 and said fix works on this branch as well, it wasn't related to the span usage.

I understand why we might not want to use spans, and am open to any suggestions of what to do instead, since AttrId's won't work because:
generating new AttrId's on decoding, like we do when decoding from rmeta for hir caching of AttrItem won't work since the query lint_expectations is arena cached, and therefore uses CacheDecoder, and making new attr items there would lead to unpredictable/non-stable sorting of the AttrId's, making them useless for comparison for expectations (I tried changing the impl of decode_attr_id for CacheDecoder to not panic and just generate AttrId, so that's how I know this)

we might be able to avoid the ICE by making the query lint_expectations not be arena cached, I will try this and update this thread (that would of course not be a long term solution)

[rust-bors]

☔ The latest upstream changes (presumably #155056) made this pull request unmergeable. Please resolve the merge conflicts.

[JonathanBrouwer]

This PR should be integrated in the new attribute lint port attempt, rather than being a separate PR
I'll see if I can dive deeper into the problems with this PR sometime in the coming days