Change the desugaring of assert! for better error output

[estebank]

In the desugaring of assert!, we now expand to a match expression instead of if !cond {..}.

The span of incorrect conditions will point only at the expression, and not the whole assert! invocation.

error[E0308]: mismatched types
  --> $DIR/issue-14091.rs:2:13
   |
LL |     assert!(1,1);
   |             ^ expected `bool`, found integer

We no longer mention the expression needing to implement the Not trait.

error[E0308]: mismatched types
  --> $DIR/issue-14091-2.rs:15:13
   |
LL |     assert!(x, x);
   |             ^ expected `bool`, found `BytePos`

Now assert!(val) desugars to:

match val {
    true => {},
    _ => $crate::panic::panic_2021!(),
}

Fix #122159.

[rustbot]

r? @pnkfelix

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

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

cc @rust-lang/rust-analyzer

The Miri subtree was changed

cc @rust-lang/miri

[estebank]

Sigh, clippy shows at least one test where a suggestion causes there to be a condition that isn't a bool but rather a type where Not returns a bool. We can easily continue supporting that by making the desugaring !!cond, but would rather not do that. This happens when it suggest changing assert_eq!(non_bool, true), ending up as assert!(non_bool), which doesn't work with this change .

[pnkfelix]

Isn't this technically a breaking change for e.g. (playground):

struct Booly(i32);

impl std::ops::Not for Booly {
    type Output = bool;
    fn not(self) -> Self::Output {
        self.0 == 0
    }
}

fn main() {
    assert!(Booly(1), "booly booly!")
}

[pnkfelix]

At the very least, we might need to tie such a change to an edition.

I am not certain whether this decision would be a T-lang matter or a T-libs-api one. I'll nominate for T-lang for now.

(Namely: The question is whether we can start enforcing a rule that the first expression to assert! must be of bool type, which is how the macro is documented, but its current behavior is a little bit more general, as demonstrated in my prior comment)

@rustbot label +I-lang-nominated

[estebank]

@pnkfelix we can keep the current (undocumented) behavior by making the desugaring be

{
    let x: bool = !!condition;
    x
}

instead of what this PR does:

{
    let x: bool = condition;
    x
}

I believe that would still cause errors to complain about Not not being implemented, instead of the more straightforward type error, albeit with a better span. I don't particularly like the idea of keeping the current emergent behavior if there aren't people exploiting it in crates.io.

Edit: an option would be to have an internal marker trait:

use std::ops::Not;
trait CanAssert {}
impl<T: Not<Output = bool>> CanAssert for T {}

fn main() {
    let _ = Box::new(true) as Box<dyn CanAssert>;
    let _ = Box::new(42) as Box<dyn CanAssert>;
}

error[E0271]: type mismatch resolving `<i32 as Not>::Output == bool`
 --> src/main.rs:7:13
  |
7 |     let _ = Box::new(42) as Box<dyn CanAssert>;
  |             ^^^^^^^^^^^^ expected `bool`, found `i32`
  |
note: required for `i32` to implement `CanAssert`
 --> src/main.rs:3:29
  |
3 | impl<T: Not<Output = bool>> CanAssert for T {}
  |             -------------   ^^^^^^^^^     ^
  |             |
  |             unsatisfied trait bound introduced here
  = note: required for the cast from `Box<i32>` to `Box<dyn CanAssert>`

[pnkfelix]

@estebank what about making the expansion edition-dependent? Is there precedent for that?

Then, editions >= 2024 would expand to what you have proposed in the code of this PR, and editions < 2024 could expand to the !!condition variant form that you have discussed in the comments?

[pnkfelix]

what about making the expansion edition-dependent? Is there precedent for that?

(to answer my own question, panic! is one obvious precedent here. So it seems like making it edition-dependent would be one acceptable path; no opinion yet as to which is best...)

[rustbot]

Some changes occurred in coverage tests.

cc @Zalathar

[estebank]

I tried the marker trait approach for <=2021, and it kind of worked, but the diagnostics were actually worse than just doing { let x: bool = !!$expr; x }, which accounts for pretty much everything we currently support, but with better spans and better errors (if typeof($expr) implements <Not<Output = NotBool>, we now produce an appropriate E0308 type error).

[compiler-errors]

Since I don't think it's been acknowledged above, for the record, this breaks the following code:

fn hello(x: &bool) {
  assert!(x);
}

Because &bool: Not<Output = bool>.

[estebank]

@compiler-errors that is indeed the case for 2024 onwards, not for previous editions.

[pnkfelix]

@compiler-errors that is indeed the case for 2024 onwards, not for previous editions.

I think the critical point is whether an edition-dependent expansion is worth breaking that case (of assert!(x) where x: &bool), or if we should do a non-breaking non-edition-dependent expansion using the let x: bool = !!$expr trick across the board...

---

Update: I don't know whether it is worth going through this exercise explicitly, but there is a design space here. E.g. one set of options is:

1. (stable Rust behavior): in all editions, support arbitrary impl Not<Output=bool> for first parameter to assert!;
2. in edition >= 2024, support just Deref<Target=bool> for first parameter to assert! (e.g. by expanding to let x: &bool = &$expr;), or
3. (this PR): in edition >= 2024, support just bool for first parameter to assert!.

(And then there's variations thereof about how to handle editions < 2024, but that's a separate debate IMO.)

[pnkfelix]

(this is waiting for a decision from T-lang and/or T-libs regarding what interface we want to commit to for assert!)

@rustbot label: +S-waiting-on-team -S-waiting-on-review

[estebank]

Thank you to everyone who spent time reviewing this PR!

[Mark-Simulacrum]

Per #145423 (comment), it looks like this caused a meaningful regression to ripgrep builds... about 0.8% slower:

My guess is that ripgrep contains a particularly large amount of assertions compared to our other primary benchmarks and so it's most affected, but presumably the new expansion is harder on the compiler? It looks like the MIR produced is ~identical between the changed expressions if I correctly skimmed the desugaring godbolt, so it's probably the earlier stages of the compiler that are affected. The match does seem like it consumes more blocks etc.

I'm guessing it's not worth digging too much deeper here, so probably can be marked as triaged. Runtime performance isn't likely to be affected given the equivalent MIR which matters more for this change IMO.

[durin42]

We've discovered that https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=81a747b4884e11637c33beeb1de28465 compiles in stable, but not after this PR. Do you want a tracking bug for that?

[samueltardieu]

We've discovered that https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=81a747b4884e11637c33beeb1de28465 compiles in stable, but not after this PR. Do you want a tracking bug for that?

I believe this was discussed in #122661 (comment) and later comments.

[estebank]

@durin42 could you point us to where that change in behavior affected you? We tried our best at doing our due diligence to avoid that behavior affecting the ecosystem, but if there was a failure in our methodology it is best to know about it so that we can revise our process in for future changes.

The release notes will highlight this behavioral change.

[durin42]

We saw it in the bitvec crate. ferrilab/bitvec#298 was filed before we realized that backing out this PR let us build bitvec again.

Sounds like we should locally-patch bitvec?

[estebank]

I'm shocked this wasn't picked up by our crater runs :-/

We had an earlier proposed approach to restrict the new desugaring to the latest edition, which would have helped/masked this one case. I am concerned that there might have been more cases out there.

[saethlin]

I'm shocked this wasn't picked up by our crater runs :-/

bitvec is prepare-fail in at least the second crater run above, along with 135,940 other crates.

https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/crater.3A.20.3E100k.20.22prepare-fail.22.20crates.3F

[Skgland]

I'm shocked this wasn't picked up by our crater runs :-/

bitvec is prepare-fail in at least the second crater run above, along with 135,940 other crates.

https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/crater.3A.20.3E100k.20.22prepare-fail.22.20crates.3F

This scenario was basically what my addition of the prepare-fail category was trying to prevent.
By moving spurious failures during crate preparation into their own category and including that category in the retry-regeressed-list.txt they were supposed to be included when a retry is done to prevent the spurious failure from masking an actual regression. Though that obviously only works if the crater run is actually retried with the retry-regressed-list.txt .