typeck: taint if errors found during writeback

[davidtwco]

Fixes #112824.
Fixes #112630.

If any type added to the TypeckResults references an error then set tainted by errors. By doing this, const eval can be skipped on bodies that reference errors which would make const eval ICE.

[rustbot]

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[compiler-errors]

@rustbot author

[davidtwco]

Okay - I've taken a different approach to this after discussions on Zulip and some thinking. I'm not sure it's ideal but haven't thought of anything better at the moment.

[davidtwco]

I can't reproduce these failures locally.

[lqd]

(it's just #113585)

[oli-obk]

Okay - I've taken a different approach to this after discussions on Zulip and some thinking. I'm not sure it's ideal but haven't thought of anything better at the moment.

I think it seems like an ok fix for now if it doesn't cause us any cycle errors in otherwise fine code. It shouldn't, as the second level isn't using the deep references error check, so only immediately recursive data structures will get hit, which seems fine.

[oli-obk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit dc85120 with merge 4296df04a725855fece448e5e4532195b8577909...

[bors]

☀️ Try build successful - checks-actions
Build commit: 4296df04a725855fece448e5e4532195b8577909 (4296df04a725855fece448e5e4532195b8577909)

[oli-obk]

@rust-timer build 4296df04a725855fece448e5e4532195b8577909

[oli-obk]

@rust-timer build 4296df0

[rust-timer]

Finished benchmarking commit (4296df04a725855fece448e5e4532195b8577909): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.5%, 1.0%]	4
Regressions ❌ (secondary)	14.0%	[3.2%, 22.0%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	0.8%	[0.5%, 1.0%]	4

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.2%	[1.0%, 3.4%]	2
Regressions ❌ (secondary)	10.7%	[1.2%, 20.8%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[1.0%, 3.4%]	2

Binary size

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

Bootstrap: 656.437s -> 657.475s (0.16%)

[oli-obk]

Hmm... I guess we could try caching the error flag in AdtFlags and making TypeFlags fetch that for the error TypeFlag instead of actually getting the field types during typeck.

[davidtwco]

Hmm... I guess we could try caching the error flag in AdtFlags and making TypeFlags fetch that for the error TypeFlag instead of actually getting the field types during typeck.

An update to what we've been trying:

• It isn't possible to do what @oli-obk has suggested above, we can't know the types of the fields when AdtFlags is being computed without running into cycle errors.
  • Just as with the current approach, calls to <ItemCtxt as AstConv>::set_tainted_by_errors would still just be ignored, so there could be other issues being missed.
• We've also tried changing EarlyBinder to contain a tainted: Option<ErrorGuaranteed> field, and require handling that error to access the type bound by EarlyBinder - this, unsurprisingly, requires an ungodly number of changes to the compiler. Just adding additional functions which do substitution and error handling, then threading that up where required to solve the current issue, results in a large number of changes.
  • Here's a gist with an example of some of these changes - enough to fix two of the tests in this PR.
  • This approach is unwieldy but - implemented fully - it does guarantee that any calls to <ItemCtxt as AstConv>::set_tainted_by_errors are handled and not ignored.
    • Around 30 tests fail if this function panics, not including those cases when the call doesn't come from type_of/would taint an EarlyBinder.

[compiler-errors]

this still needs work, perhaps even a more hands-on discussion? idk

@rustbot author

[Dylan-DPC]

@davidtwco any updates on this? thanks

[JohnCSimon]

@davidtwco ping from triage: any updates on this? thanks

[davidtwco]

No updates, this still needs fixed but we don't have a path forward at the moment.

[alex-semenyuk]

@davidtwco
From wg-triage. Do you have path forward on this? Is it blocked on some discussion?

[davidtwco]

Blocked on working out how we'd like to fix this