Conditionally compile contracts instead of deciding at run-time

[dawidl022]

The initial implementation of contracts, despite requiring a compiler flag to enable runtime checking of contracts, still compiled contracts into function definitions, even when the compiler flag was disabled. This meant that contracts could not be safely added to functions without breaking optimisations, or even without potentially changing the behaviour of the function. This was blocking the addition of contracts to standard library functions in #136578.

This change guards macro expansion of the built-in contract macros with the contract-checks compiler flag. Additionally, it removes the contract_checks compiler intrinsic that was used to determine whether contract checks should be executed at runtime. Now, when contracts checks are compiled into the body of a function, they will always be executed.

The change is motivated by the following discussion: #144438 (comment)

Contracts tracking issue: #128044

Known limitations:

• When contract-checks are disabled, contracts will not be parsed or type checked, meaning that they are susceptible to becoming out of sync with the rest of the codebase.

[rustbot]

r? @lcnr

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

[rustbot]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

[saethlin]

This was blocking the addition of contracts to standard library functions in #136578.

Hunh? All I can see in that PR is contract checks changing the optimized MIR of functions which is expected and should be fine. Disabled contract check MIR should be cleaned out as part of lowering to LLVM, or it should be immediately cleaned up by LLVM because it is under the equivalent of an if false.

[dawidl022]

This was blocking the addition of contracts to standard library functions in #136578.

Hunh? All I can see in that PR is contract checks changing the optimized MIR of functions which is expected and should be fine. Disabled contract check MIR should be cleaned out as part of lowering to LLVM, or it should be immediately cleaned up by LLVM because it is under the equivalent of an if false.

How does one evaluate whether disabled contract check code is indeed cleaned up? You mentioned in #136578 (comment):

I suspect you'll then fail codegen tests, and those may be much more troublesome.

Could you elaborate on this please? Indeed one of the codegen tests (tests/codegen/cross-crate-inlining/leaf-inlining.rs) fails on that branch, and I don't have the expertise to judge what the consequences of that test failure are.

I feel that contracts should not interfere with optimisations of the code in any way, since to me they serve as an executable piece of documentation, comparable to rustdoc tests. Having contracts get in the way of normal code will likely slow down their adoption.

[lcnr]

I haven't been involved in the contracts work, so it might make sense to reassign this to someone who is more involved in that effort.

I would expect that long term, we definitely want contracts to be type checked even if we don't actually enforce them at runtime, is that correct?

I would also expect that if false in the MIR should get optimized out without negatively impacting other optimizations. If not, this just seems like a generally desirable MIR optimization?

[dawidl022]

r? @celinval

I would expect that long term, we definitely want contracts to be type checked even if we don't actually enforce them at runtime, is that correct?

Yes, I believe that is the case.

I would also expect that if false in the MIR should get optimized out without negatively impacting other optimizations. If not, this just seems like a generally desirable MIR optimization?

Agreed, it sounds desirable. I'm less sure however about relying on optimisations, when we can simply not compile the contracts when disabled. E.g. I'm not sure what the compile-time penalty might be of optimising out contracts compared to excluding them from compilation in the first place.

[lcnr]

E.g. I'm not sure what the compile-time penalty might be of optimising out contracts compared to excluding them from compilation in the first place.

Excluding them from compilation must only happen post MIR borrowck/after analysis as it shouldn't be user-facing, should it not?

At this point removing them has to be a MIR optimization pass, so it can just be a more general "remove if false" pass which runs after const folding

[dawidl022]

Excluding them from compilation must only happen post MIR borrowck/after analysis as it shouldn't be user-facing, should it not?

And the advantage of this approach is that contracts stay type-checked, or was there some other advantage(s) you had in mind?

[saethlin]

this just seems like a generally desirable MIR optimization?

If I recall correctly, contract checks have the same problem as the UB checks; whether they are enabled is not always known until after monomorphization. So this needs a post-mono MIR optimization.

[lcnr]

Excluding them from compilation must only happen post MIR borrowck/after analysis as it shouldn't be user-facing, should it not?

And the advantage of this approach is that contracts stay type-checked, or was there some other advantage(s) you had in mind?

Not really. It's less about this being an advantage rather than "it being a MIR opt is unavoidable if we want stuff to get typechecked". If we want them to be type checked, they need to be in the MIR until after MIR borrowck. The only thing:tm: happening after MIR borrowck are MIR optimizations and codegen.

If I recall correctly, contract checks have the same problem as the UB checks; whether they are enabled is not always known until after monomorphization. So this needs a post-mono MIR optimization.

In that case removing them syntactically also doesn't help however, does it?

[saethlin]

In that case removing them syntactically also doesn't help however, does it?

The point of enabling/disabling checks at mono-time is to permit use of those checks without recompiling the standard library. As far as I'm aware, this is a highly desirable property. I certainly found it quite effective with the runtime UB checks, and I thought the contract checks work was following the same strategy for the same reasons. This approach must regress optimized_mir which is what the latest discussion in the referenced PR #136578 was about. As far as I'm aware, that PR is blocked on fighting with the mir-opt test suite and it does not have the problems that this PR description suggests that it has.

This PR does fix the problems that OP says the other one has, but at the cost of making contract checks in the standard library only usable via nightly-only opt-in, and I'm just confused by that tradeoff because I don't believe the motivating problem is real.

[celinval]

I suggested the removal of the contracts_check intrinsic for now. I do think it should simplify things as part of the initial experimentation and design work. We still have a lot of open questions that I would like to solve before fighting the mir-opt test suite and issues around side effects.

I agree that for wider adoption of std library contracts we will need to implement an easier way for std users to enable them.

[tautschnig]

@saethlin said:

[...] This approach must regress optimized_mir which is what the latest discussion in the referenced PR #136578 was about. As far as I'm aware, that PR is blocked on fighting with the mir-opt test suite and it does not have the problems that this PR description suggests that it has.

Given #136578 (comment) - how come we get different outcomes for aarch64 vs x86_64 if the stages are working as you suggest? If we are getting MIR before mono-time then I'd expect it to be the same across architectures for the same contracts are used irrespective of the platform. If it is past mono-time then there shouldn't be any contracts left so, again, the test outcomes should not have changed.

[saethlin]

how come we get different outcomes for aarch64 vs x86_64

Reproduce that difference outside of the mir-opt test suite by using --emit=mir before supposing that the problem is actually the architecture, as opposed to the test suite's strange semantics.

[RalfJung]

FWIW I think it's fine to make the contracts nightly-only for now -- hopefully cargo build-std makes progress Soon (TM) which would then provide a less difficult way forward for having these checks available on stable.

However, having them type-checked still sounds desirable. I think it'd be reasonable for this to behave like debug_assert!.

[celinval]

FWIW I think it's fine to make the contracts nightly-only for now -- hopefully cargo build-std makes progress Soon (TM) which would then provide a less difficult way forward for having these checks available on stable.

However, having them type-checked still sounds desirable. I think it'd be reasonable for this to behave like debug_assert!.

@dawidl022 I know you bumped into some issues in your first attempt. Do you still have the code for that? I agree with @RalfJung that we should still type check the contract code.

[dawidl022]

@dawidl022 I know you bumped into some issues in your first attempt. Do you still have the code for that? I agree with @RalfJung that we should still type check the contract code.

Yes, the code was a single-line change in the lowering code: dawidl022@f341db4

However, since this basically guarded HIR lowering of contracts, they would still not be type checked --- the only advantage of doing it at the lowering stage is that they will at least be syntax checked (i.e. parsed).

I'm happy to investigate debug_assert! to see if I can get some ideas based on that implementation.

[RalfJung]

I'm happy to investigate debug_assert! to see if I can get some ideas based on that implementation.

It's nothing fancy, just if cfg!(...) so by the time we reach the MIR, it's if true/if false.

[celinval]

I added a comment in your other CR. Let me know if that makes sense.

[saethlin]

@Connor-GH Please do not comment on issues/PRs to interact with AI. The tracking issue linked in the PR description has a lot of background context.

[celinval]

@dawidl022 can we close this PR in favor of #144438?

[dawidl022]

@dawidl022 can we close this PR in favor of #144438?

Yes, I'm happy to close this PR once #144438 is merged :)

[Dylan-DPC]

i'm going to close this pr because of the other pr. If that doesn't get merged we can always reöpen this one.