do not eagerly use Reveal::All in const_eval_resolve_for_typeck

[lcnr]

We should not use RevealAll when resolving the instance; only when evaluating its body.

r? @BoxyUwU

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[lcnr]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 780bd2f with merge 8b79e04...

[lcnr]

anything that cares about infer vars should use infcx.try_const_eval_resolve :<

[bors]

☀️ Try build successful - checks-actions
Build commit: 8b79e04 (8b79e04e23ef8433b4d1e41d936d3ed8e1173faf)

[rust-timer]

Finished benchmarking commit (8b79e04): comparison URL.

Overall result: ❌ regressions - please read the text below

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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	3

Max RSS (memory usage)

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

Cycles

Results (secondary -6.1%)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.1%	[-6.1%, -6.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 784.059s -> 782.369s (-0.22%)
Artifact size: 333.59 MiB -> 333.45 MiB (-0.04%)

[BoxyUwU]

I like the general cleanup in this PR though I think it (unfortunately) makes sense to continue using a Reveal::All env for resolving instances when evaluating const arguments. We already use Reveal::All unconditionally when evaluating the body which means that we handle opaques "incorrectly" regardless.

Since getting an instance of an anon const will trivially succeed regardless of if it contains any opaque types, that means that not using Reveal::All introduces the possibility to have a const argument that fails to resolve (and therefore be evaluated) while it succeeding if wrapped in an anon const.

One thing we could do to avoid that would be to not reveal opaque types during ctfe in typeck but that seems like a far wider change (and also one that is perhaps not super desirable).

[lcnr]

this is kinda irrelevant now that #132927 has landed, closing