Add an experimental internal attribute for limiting const evaluation to crate-local things

[oli-obk]

View all comments

As a part of rust-lang/goals#620, we'll want to be able to run const eval on MIR created (transitively, through the usual steps) from local AST, but not allow it to access anything outside that body. This is fairly straight forward for many things like accessing structs/functions from the local crate, as we can just handle that at resolving time (or it will automatically happen, as we literally can't see the other items existing). But there are global things like trait solving, where a trait from an upstream crate can be implemented for a type from the current crate, and the compiler will first collect all the impls from all crates when looking at a trait in the trait solver.

This PR adds the forever unstable rustc_isolated_const attribute that changes the solver mode to IsolatedConst, which in turn just doesn't collect any of the local impls at all. This is the most minimal step towards avoiding a query dependency from hir_ty_lowering, typeck, borrowck, and ctfe to the global resolver query. There are more steps necessary, but this PR adds the framework and performs the most visible limitations.

Thanks @scrabsha for doing the major part of the work here and passing it on to me

r? types

[rustbot]

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred to the CTFE machinery

cc @RalfJung, @lcnr

Some changes occurred in compiler/rustc_attr_ir

cc @jdonszelmann, @JonathanBrouwer

HIR ty lowering was modified

cc @fmease

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[oli-obk]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5682a2d (5682a2d90b87b2f67b00147c28c8e7b52c2b2c3a)
Base parent: d287eb7 (d287eb7a292caa8abbe12051dce2ee707fe03200)

[rust-timer]

Finished benchmarking commit (5682a2d): comparison URL.

Overall result: ❌ regressions - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	3
Regressions ❌ (secondary)	0.4%	[0.1%, 0.8%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.6%]	1
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	3

Max RSS (memory usage)

Results (primary -2.2%, secondary 6.9%)

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)	6.9%	[6.9%, 6.9%]	1
Improvements ✅ (primary)	-2.2%	[-3.3%, -1.1%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.2%	[-3.3%, -1.1%]	2

Cycles

Results (secondary 7.8%)

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

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 503.997s -> 496.497s (-1.49%)
Artifact size: 408.93 MiB -> 406.90 MiB (-0.50%)

[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.

[lcnr]

what, why?

View changes since the review

[oli-obk]

XD I was thinking I should add a comment here... typeck needs to know about IsolatedConst as well, but that would mean essentially adding a new dimension typing mode, where everything can be an isolated const, but also have the normal typingmode.

Instead this MVP just forbids having opaque types at all, even if we could make RPITs work. TAITs won't work as they require access to other items from the current crate

[lcnr]

when do we use all_impls without specifying the typing mode? because if so, there should maybe just be an infcx.all_impls wrapper which considers the typing mode

View changes since the review

[oli-obk]

we should never use all_impls without specifying the mode, and always use the mode of the infcx if available

[lcnr]

same here, feel like a lot of these matches should be bug! instead?

or well, should this be a bug!, maybe not

View changes since the review

[lcnr]

this comment needs a lot more info. Also, the PR title is wrong? isn't this limiting ctfe to non-local things?

View changes since the review

[oli-obk]

the intent is to have CTFE run while only depending on non-local things. We need to prevent the trait solver from accessing things from the current crate, too (I typoed crate and wrote trait in the comment here)

[RalfJung]

This doesn't actually check that the functions invoked during CTFE are all from the current crate, does it?

View changes since the review

[oli-obk]

That's not important in practice for the end goal. Either the resolver doesn't even let you call that function, or you get a cycle error, or you can call that function. All the work here is to avoid cycle errors, and we'll see how far we can take this

[RalfJung]

limiting const evaluation to crate-local things

I read this as "only allow crate-local things". But looking at the goal it seems you mean the opposite?

[lcnr]

This currently does not handle nested queries during CTFE, e.g. impossible_predicates. Effectively any query which takes a def_id and does trait solving (i.e. everything that does TypingEnv::fully_monomorphized() can cause query cycles here)

Similarly, just doing it for CTFE is insufficient as we also need to do the same during all queries affecting a given body, e.g. typeck, borrowck, and so on. This means that just a single typingmode is not enough and we need something more of a TypingMode x MayAccessUpstream.

Given the way this is incredibly pervasive, it might even make sense to integrate support for this in the query system itself, as we likely do want to cache results between these queries if they don't actually depend on anything local and as explicitly tracking this flag is a very invasive change.

Having some uses of the trait solver be able to access all impls while others only access foreign ones also makes me somewhat worried about things being inconsistent/unsound, e.g. impossible_predicates could encounter ForeignType: Trait<LocalTy> and consider it to be impossible, even though it can be implemented in the current crate. Effectively only limiting impls, but not types, should be unsound. This means it may also be nice to integrate it into the TyCtxt and maybe even add asserts that one cannot access LocalDefIds while in this mode or sth

[rust-bors]

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