Deny calls to non-#[const_trait] methods in MIR constck

[fee1-dead]

This is a (potentially temporary) fix that closes off the mismatch in assumptions between MIR constck and typeck which does the const traits checking. Before this PR, MIR constck assumed that typeck correctly handled all calls to trait methods in const contexts if effects is enabled. That is not true because typeck only correctly handles callees that are const. For non-const callees (such as methods in a non-const_trait), typeck had never created an error.

rust/compiler/rustc_hir_typeck/src/callee.rs

Lines 876 to 877 in 45089ec

	// FIXME(effects): This should eventually be caught here.
	// For now, though, we defer some const checking to MIR.

I called this potentially temporary because the const checks could be moved to HIR entirely. Alongside the recent refactor in const stability checks where that component could be placed would need more discussion. (cc @compiler-errors @RalfJung)

Tests are updated, mainly due to traits not being const in core, so tests that call them correctly error.

This fixes rust-lang/project-const-traits#12.

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
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

[compiler-errors]

I'm not totally convinced we should be tweaking const checking here in favor of just reimplementing it the right way from scratch.

So I think the right approach would be to 1. check the const conditions of every conditionally const item (including free functions with ~const bound), and eventually delay a bug if they fail (this could for now only be done on CallSource::Normal perhaps) and 2. also make sure that const traits are only callable with the const trait feature enabled.

Additionally, the Instance::resolve call should be totally deleted, since enforcing const stability on resolved items is not really feasible given trait resolution may only succeed on monomorphic code.

Not totally certain why we're patching the issue partially here if it doesn't necessarily lead us closer to the right approach at the end. I think I expressed that I had been working on a rework of const stability checking that addresses these thoughts, but maybe I didn't make that super clear 😅

[compiler-errors]

@rustbot author

[fee1-dead]

So I think the right approach would be to 1. check the const conditions of every conditionally const item (including free functions with ~const bound), and eventually delay a bug if they fail (this could for now only be done on CallSource::Normal perhaps) and 2. also make sure that const traits are only callable with the const trait feature enabled.

1 will be achieved through enforce_context_effects once we remove the effects feature. 2 is already currently done in MIR constck.

This isn't patching the issue partially because typeck does check const conditions properly, the issue here is that there are no const conditions to check. (because callee is not in a const trait)

Denying non-const calls has always been done in MIR constck, so it makes sense to fix denying calls to non-const trait methods here as well. typeck is (atm) only responsible for making sure that const conditions for maybe-const items are satisfied. The separation of responsibilities is pretty clear to me. Therefore if you wanted typeck to take on the role of stability checking and denying non-const calls, that would be orthogonal to this PR.

Happy to chat more on Zulip as well.

[fee1-dead]

(and yes, checking stability of concrete impls like that is quite broken as you said, but this PR is about rejecting things we should be rejecting and not fixing the const stability checks)

[compiler-errors]

I guess I'll just change the way we do const stability enforcement on top of this. I'd still like to see three things fixed in this PR:

1. Rework all of that logic starting with // Check that all trait bounds that are marked as ~const can be satisfied. -- it's no longer true and no longer doing anything, since ~const bounds are not even in the predicates_of anymore. Ideally we'd adjust that logic to register and check const_conditions for all conditionally const items, and keep that as a failsafe for making sure we don't accidentally forget to check ~const bounds in HIR, given that many different HIR operations can translate into call terminators during MIR lowering. That's what I meant by (1.) above, not that HIR typeck shouldn't be primarily responsible for enforcing constness.
2. Remove that call to Instance::try_resolve call, since it's really not conceptually well-founded; we shouldn't be specializing constness checking to specific impl items, since this is both inconsistent and unnecessary IMO, and easily side-stepped with things like trivial where clause and indirection via generics.
3. It would be very nice if you tried to despaghettify the existing logic. There's a lot of nesting of thes if statements and I'm not totally certain that it should exist. @RalfJung had an idea, and this is kinda what I mean by (2.) above, but ideally we'd uplift the const enforcement of trait methods to a new NonConstOp implementation and enforce const trait calls via that (i.e. override its status with Status::Unstable(sym::const_traits)). I'm not sure why we even need to care about the effects feature gate in MIR checking, since IMO that should (at least for now, obviously being removed later) gate just the enforcement of effects in HIR typeck -- MIR should enforce these unconditionally.

[compiler-errors]

If you think that's overkill, that's fine, I'd like to hear your thoughts; my justification here is basically that I feel like nothing about fixing rust-lang/project-const-traits#12 is super urgent, and as long as we're touching this const stability checking now, why not clean up the parts that are obviously broken.

But if you don't want to do that, then I guess we can probably just land this PR as is, and those other parts can be done later.

[compiler-errors]

nvm, discussed offline. i'll just work on these follow-ups separately.

[compiler-errors]

@bors r+ rollup

[bors]

📌 Commit f2f6723 has been approved by compiler-errors

It is now in the queue for this repository.