Implement RFC-2011 (Nicer `assert!` messages) #96496

c410-f3r · 2022-04-28T01:07:22Z

Functionality

The intention here is to show the contents of variables that are outside the declaration of assert!( ... ).

Current behavior

fn main() {
    let a = 1;
    let b = true;
    assert!(a == 1 && !b);
}

// Output:
//
// assertion failed: a == 1 && !b

New behavior with this PR

#![feature(generic_assert)]

fn main() {
    let a = 1;
    let b = true;
    assert!(a == 1 && !b);
}

// Output:
//
// Assertion failed: a == 1 && !b
// With captures:
//  a = 1
//  b = true

Internals

All external assert! variables are replaced by a block that tries to copy the intended value and then use this possibly copied element to print useful messages if a branch occurs.

fn main() {
    #[derive(PartialEq)]
    struct NoDebug(i32);
    
    let no_debug = NoDebug(1);
    let number = 3i32;
    
    assert!(no_debug != NoDebug(2) && 1 == number);
}

With this reasoning in mind, the above assert! will more or less expand to the following code:

#[derive(PartialEq)]
struct NoDebug(i32);

let no_debug = NoDebug(1);
let number = 3i32;

{
    let mut __capture0 = Capture::new();
    let mut __capture1 = Capture::new();

    let __local_bind0 = &no_debug;
    let __local_bind1 = &number;

    if !(
        // no_debug != NoDebug(2)
        *{
            (&Wrapper(__local_bind0)).try_capture(&mut __capture1);
            __local_bind0
        } != NoDebug(2)
        // 1 == number
        && 1 == *{
            (&Wrapper(__local_bind1)).try_capture(&mut __capture2);
            __local_bind1
        }
    ) {
        panic!(
            "Assertion failed: no_debug != NoDebug(2) && 1 == number\nWith captures:\n  no_debug = {:?}\n  number = {:?}",
            __capture0,
            __capture1,
        );
    }
}

The compiler "side" outputs the necessary machinery to make the library "side" decide how an element T will or will not be printed.

Performance

It is inevitable that more instructions will be issued but probably nothing that will have a huge negative impact. Nevertheless, it is possible to optimize certain scenarios to reduce codegen.

; https://godbolt.org/z/94rE4dE4a (worst-case)
example::with_generic_assert:
        sub     rsp, 104
        mov     dword ptr [rsp + 16], 0
        mov     dword ptr [rsp + 8], 0
        cmp     edi, 2
        je      .LBB2_3
        mov     dword ptr [rsp + 8], 1
        mov     dword ptr [rsp + 12], esi
        cmp     esi, 1
        jne     .LBB2_3
        add     rsp, 104
        ret

; https://godbolt.org/z/P8T4rboac (best-case)
example::with_generic_assert:
        sub     rsp, 104
        mov     dword ptr [rsp + 16], 0
        cmp     edi, 2
        je      .LBB2_3
        cmp     esi, 1
        jne     .LBB2_3
        add     rsp, 104
        ret

example::without_generic_assert:
        cmp     edi, 2
        je      .LBB3_3
        cmp     esi, 1
        jne     .LBB3_3
        ret

rust-highfive · 2022-04-28T01:07:25Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-04-28T01:07:26Z

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

c410-f3r · 2022-04-28T01:07:37Z

r? @petrochenkov

Opening early as we discussed but the compiler side is a bit of a mess, specially the from_assert_expr_to_expr method. Everything inside context.rs need to be reformulated into a more cleaner and more efficient architecture.

~~That said, feel free to review this PR now or when the code becomes more robust (probably next week)~~

I guess the code is now ready for review

thomcc · 2022-04-28T04:46:29Z

We should check the performance impact of this once it's ready (I guess there's more to do?). I know that I've seen large differences in tight loops between assert!(a == b) and assert_eq!(a, b) due to the format machinery (although neither are as good as manually outlining it).

I don't think this is a blocker (the functionality is very nice) but once yeah, it probably warrants a perf run.

clarfonthey · 2022-04-28T05:00:28Z

Shouldn't the format machinery not affect the performance of the assert unless it fails? Or is there some weird stuff going on in the codegen?

c410-f3r · 2022-04-28T11:14:04Z

Unfortunately, I can not answer all the questions because this PR is still a work in progress. Performance heavily depends on codegen but I will try to do my best to make everything work as fast as possible.

c410-f3r · 2022-04-29T01:11:26Z

Reviewing should be easier now that the code is in a much better shape

thomcc · 2022-04-29T01:25:32Z

It is inevitable that more instructions will be issued but probably nothing that will have a huge negative impact. Moreover, it is possible to optimize certain scenarios to reduce codegen.

This helps, but there is significantly more codegen not shown in the example -- this impacts performance due to icache usage even if the branch is not taken. Anyway, as I said, I don't think this blocks things, but I do think we should be aware of the performance impact of this (which may be small... and admittedly, possibly not measurable by our tools, which go by instruction counts, and thus will not judge things like icache impact), so I think we should do a timer run.

@bors try @rust-timer queue

rust-timer · 2022-04-29T01:25:34Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-04-29T01:25:40Z

⌛ Trying commit eae545d20fd3e6aa6579f87ee989c5125340beaa with merge f91eeed48dde360eaa79c339591316da5fa20226...

c410-f3r · 2022-04-29T01:46:08Z

@thomcc The new code is behind a feature called generic_assert that is disabled by default. I can enable the feature for measurement but right now a perf run probably won't show anything different

thomcc · 2022-04-29T02:08:26Z

Shoot.

nbdd0121 · 2022-04-29T14:46:53Z

How does this interact with panicking in const contexts?

c410-f3r · 2022-04-29T17:37:59Z

How does this interact with panicking in const contexts?

With the feature disabled, nothing changes.

With the feature enabled, I am not sure :)
I guess ::core::asserting::* need all the necessary unstable constant trait bounds to properly work. If so, I will tackle everything in a following PR.

compiler/rustc_builtin_macros/src/assert/context.rs

petrochenkov · 2022-05-20T20:10:03Z

FWIW, implementation of this and many other built-in macros can be simplified to a usual proc macro level by implementing a version of quote! that works with rustc-internal token streams and can be used inside the compiler.

[RFC 2011] Library code CC rust-lang#96496 Based on https://github.com/dtolnay/case-studies/tree/master/autoref-specialization. Basically creates two traits with the same method name. One trait is generic over any `T` and the other is specialized to any `T: Printable`. The compiler will then call the corresponding trait method through auto reference. ```rust fn main() { let mut a = Capture::new(); let mut b = Capture::new(); (&Wrapper(&1i32)).try_capture(&mut a); // `try_capture` from `TryCapturePrintable` (&Wrapper(&vec![1i32])).try_capture(&mut b); // `try_capture` from `TryCaptureGeneric` assert_eq!(format!("{:?}", a), "1"); assert_eq!(format!("{:?}", b), "N/A"); } ``` r? `@scottmcm`

c410-f3r · 2022-06-01T10:41:17Z

r? @oli-obk

[RFC 2011] Basic compiler infrastructure Splitting rust-lang#96496 into smaller pieces as was done in rust-lang#97233. Hope review will be easier. This PR practically contains no logic and only serves as a building ground for the actual code that will be placed in a posterior step. * Adds `context.rs` to place the new `assert!` logic. Has a lot of unused elements but all of them are used by the implementation. * Creates an unstable flag because the feature is not yet complete and also to allow external feedback. * Creates the necessary `sym` identifiers that are mostly based on the library elements -> https://github.com/rust-lang/rust/blob/master/library/core/src/asserting.rs * Modifies `assert.rs` to branch to `context.rs` if the unstable flag is enabled. * Adds a test to satisfy tidy but the test does nothing in reality.

bors · 2022-06-02T19:11:53Z

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

[RFC 2011] Minimal initial implementation Tracking issue: rust-lang#44838 Third step of rust-lang#96496 Implementation has ~290 LOC with the bare minimum to be in a functional state. Currently only searches for binary operations to mimic what `assert_eq!` and `assert_ne!` already do. r? `@oli-obk`

[RFC 2011] Minimal initial implementation Tracking issue: rust-lang#44838 Third step of rust-lang#96496 Implementation has ~290 LOC with the bare minimum to be in a functional state. Currently only searches for binary operations to mimic what `assert_eq!` and `assert_ne!` already do. r? ``@oli-obk``

[RFC 2011] Minimal initial implementation Tracking issue: rust-lang#44838 Third step of rust-lang#96496 Implementation has ~290 LOC with the bare minimum to be in a functional state. Currently only searches for binary operations to mimic what `assert_eq!` and `assert_ne!` already do. r? `@oli-obk`

c410-f3r · 2022-06-15T20:43:06Z

Closing now that #97665 was merged.

@petrochenkov @scottmcm @oli-obk

Thank you all for making this RFC possible.

[RFC 2011] Expand expressions where possible Tracking issue: rust-lang#44838 Fourth step of rust-lang#96496 Extends rust-lang#97665 considering expressions that are good candidates for expansion. r? `@oli-obk`

[RFC 2011] Optimize non-consuming operators Tracking issue: rust-lang#44838 Fifth step of rust-lang#96496 The most non-invasive approach that will probably have very little to no performance impact. ## Current behaviour Captures are handled "on-the-fly", i.e., they are performed in the same place expressions are located. ```rust // `let a = 1; let b = 2; assert!(a > 1 && b < 100);` if !( { ***try capture `a` and then return `a`*** } > 1 && { ***try capture `b` and then return `b`*** } < 100 ) { panic!( ... ); } ``` As such, some overhead is likely to occur (Specially with very large chains of conditions). ## New behaviour for non-consuming operators When an operator is known to not take `self`, then it is possible to capture variables **AFTER** the condition. ```rust // `let a = 1; let b = 2; assert!(a > 1 && b < 100);` if !( a > 1 && b < 100 ) { { ***try capture `a`*** } { ***try capture `b`*** } panic!( ... ); } ``` So the possible impact on the runtime execution time will be diminished. r? `@oli-obk`

[RFC 2011] Optimize non-consuming operators Tracking issue: rust-lang#44838 Fifth step of rust-lang#96496 The most non-invasive approach that will probably have very little to no performance impact. ## Current behaviour Captures are handled "on-the-fly", i.e., they are performed in the same place expressions are located. ```rust // `let a = 1; let b = 2; assert!(a > 1 && b < 100);` if !( { ***try capture `a` and then return `a`*** } > 1 && { ***try capture `b` and then return `b`*** } < 100 ) { panic!( ... ); } ``` As such, some overhead is likely to occur (Specially with very large chains of conditions). ## New behaviour for non-consuming operators When an operator is known to not take `self`, then it is possible to capture variables **AFTER** the condition. ```rust // `let a = 1; let b = 2; assert!(a > 1 && b < 100);` if !( a > 1 && b < 100 ) { { ***try capture `a`*** } { ***try capture `b`*** } panic!( ... ); } ``` So the possible impact on the runtime execution time will be diminished. r? ``@oli-obk``

[RFC 2011] Optimize non-consuming operators Tracking issue: rust-lang#44838 Fifth step of rust-lang#96496 The most non-invasive approach that will probably have very little to no performance impact. ## Current behaviour Captures are handled "on-the-fly", i.e., they are performed in the same place expressions are located. ```rust // `let a = 1; let b = 2; assert!(a > 1 && b < 100);` if !( { ***try capture `a` and then return `a`*** } > 1 && { ***try capture `b` and then return `b`*** } < 100 ) { panic!( ... ); } ``` As such, some overhead is likely to occur (Specially with very large chains of conditions). ## New behaviour for non-consuming operators When an operator is known to not take `self`, then it is possible to capture variables **AFTER** the condition. ```rust // `let a = 1; let b = 2; assert!(a > 1 && b < 100);` if !( a > 1 && b < 100 ) { { ***try capture `a`*** } { ***try capture `b`*** } panic!( ... ); } ``` So the possible impact on the runtime execution time will be diminished. r? ```@oli-obk```

[RFC 2011] Optimize non-consuming operators Tracking issue: rust-lang#44838 Fifth step of rust-lang#96496 The most non-invasive approach that will probably have very little to no performance impact. ## Current behaviour Captures are handled "on-the-fly", i.e., they are performed in the same place expressions are located. ```rust // `let a = 1; let b = 2; assert!(a > 1 && b < 100);` if !( { ***try capture `a` and then return `a`*** } > 1 && { ***try capture `b` and then return `b`*** } < 100 ) { panic!( ... ); } ``` As such, some overhead is likely to occur (Specially with very large chains of conditions). ## New behaviour for non-consuming operators When an operator is known to not take `self`, then it is possible to capture variables **AFTER** the condition. ```rust // `let a = 1; let b = 2; assert!(a > 1 && b < 100);` if !( a > 1 && b < 100 ) { { ***try capture `a`*** } { ***try capture `b`*** } panic!( ... ); } ``` So the possible impact on the runtime execution time will be diminished. r? ````@oli-obk````

[RFC 2011] Library code CC rust-lang/rust#96496 Based on https://github.com/dtolnay/case-studies/tree/master/autoref-specialization. Basically creates two traits with the same method name. One trait is generic over any `T` and the other is specialized to any `T: Printable`. The compiler will then call the corresponding trait method through auto reference. ```rust fn main() { let mut a = Capture::new(); let mut b = Capture::new(); (&Wrapper(&1i32)).try_capture(&mut a); // `try_capture` from `TryCapturePrintable` (&Wrapper(&vec![1i32])).try_capture(&mut b); // `try_capture` from `TryCaptureGeneric` assert_eq!(format!("{:?}", a), "1"); assert_eq!(format!("{:?}", b), "N/A"); } ``` r? `@scottmcm`

rust-highfive assigned cjgillot Apr 28, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 28, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2022

rust-highfive assigned petrochenkov and unassigned cjgillot Apr 28, 2022

c410-f3r force-pushed the assert branch from e39992e to 26d2c6b Compare April 28, 2022 01:51

c410-f3r force-pushed the assert branch from 26d2c6b to eae545d Compare April 29, 2022 00:35

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 29, 2022

c410-f3r force-pushed the assert branch from eae545d to 24facc4 Compare April 29, 2022 02:04

This comment has been minimized.

Sign in to view

petrochenkov removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 29, 2022

c410-f3r force-pushed the assert branch 2 times, most recently from 066a3e7 to d8b749a Compare April 29, 2022 14:41

c410-f3r commented Apr 29, 2022

View reviewed changes

compiler/rustc_builtin_macros/src/assert/context.rs Outdated Show resolved Hide resolved

c410-f3r mentioned this pull request May 20, 2022

[RFC 2011] Library code #97233

Merged

c410-f3r added 2 commits May 25, 2022 11:05

Implementation

03cc60a

Tests

9c3d283

c410-f3r force-pushed the assert branch from 9cdb19b to 9c3d283 Compare May 25, 2022 14:05

c410-f3r mentioned this pull request May 27, 2022

[RFC 2011] Basic compiler infrastructure #97450

Merged

rust-highfive assigned oli-obk and unassigned dtolnay Jun 1, 2022

c410-f3r mentioned this pull request Jun 3, 2022

[RFC 2011] Minimal initial implementation #97665

Merged

c410-f3r closed this Jun 15, 2022

c410-f3r mentioned this pull request Jun 15, 2022

[RFC 2011] Expand expressions where possible #98148

Merged

c410-f3r mentioned this pull request Jun 21, 2022

[RFC 2011] Optimize non-consuming operators #98337

Merged

fmease added the F-generic_assert `#![feature(generic_assert)]` label Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RFC-2011 (Nicer `assert!` messages) #96496

Implement RFC-2011 (Nicer `assert!` messages) #96496

c410-f3r commented Apr 28, 2022 •

edited

Loading

rust-highfive commented Apr 28, 2022

rust-highfive commented Apr 28, 2022

c410-f3r commented Apr 28, 2022 •

edited

Loading

thomcc commented Apr 28, 2022 •

edited

Loading

clarfonthey commented Apr 28, 2022

c410-f3r commented Apr 28, 2022

c410-f3r commented Apr 29, 2022

thomcc commented Apr 29, 2022

rust-timer commented Apr 29, 2022

bors commented Apr 29, 2022

c410-f3r commented Apr 29, 2022 •

edited

Loading

thomcc commented Apr 29, 2022

This comment has been minimized.

nbdd0121 commented Apr 29, 2022

c410-f3r commented Apr 29, 2022 •

edited

Loading

petrochenkov commented May 20, 2022

c410-f3r commented Jun 1, 2022

bors commented Jun 2, 2022

c410-f3r commented Jun 15, 2022

Implement RFC-2011 (Nicer assert! messages) #96496

Implement RFC-2011 (Nicer assert! messages) #96496

Conversation

c410-f3r commented Apr 28, 2022 • edited Loading

Functionality

Current behavior

New behavior with this PR

Internals

Performance

rust-highfive commented Apr 28, 2022

rust-highfive commented Apr 28, 2022

c410-f3r commented Apr 28, 2022 • edited Loading

thomcc commented Apr 28, 2022 • edited Loading

clarfonthey commented Apr 28, 2022

c410-f3r commented Apr 28, 2022

c410-f3r commented Apr 29, 2022

thomcc commented Apr 29, 2022

rust-timer commented Apr 29, 2022

bors commented Apr 29, 2022

c410-f3r commented Apr 29, 2022 • edited Loading

thomcc commented Apr 29, 2022

This comment has been minimized.

nbdd0121 commented Apr 29, 2022

c410-f3r commented Apr 29, 2022 • edited Loading

petrochenkov commented May 20, 2022

c410-f3r commented Jun 1, 2022

bors commented Jun 2, 2022

c410-f3r commented Jun 15, 2022

Implement RFC-2011 (Nicer `assert!` messages) #96496

Implement RFC-2011 (Nicer `assert!` messages) #96496

c410-f3r commented Apr 28, 2022 •

edited

Loading

c410-f3r commented Apr 28, 2022 •

edited

Loading

thomcc commented Apr 28, 2022 •

edited

Loading

c410-f3r commented Apr 29, 2022 •

edited

Loading

c410-f3r commented Apr 29, 2022 •

edited

Loading