Add generic_assert by ishitatsuyuki · Pull Request #2011 · rust-lang/rfcs (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation38 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

euclio, kornelski, aturon, manuel-woelker, birkenfeld, Robbepop, ShikChen, DaseinPhaos, marhoily, behnam, and 3 more reacted with thumbs up emoji stepancheg reacted with thumbs down emoji HadrienG2 and jdm reacted with hooray emoji

Thoughts:

We can't fail to compile if intermediate portions of the expression can't be stringified as people are already potentially using assert on things which can be compared but not stringified. This would be a breaking change.

If Rust asserts are checked in release builds, the performance hit is a problem. I'm not sure if they are or not. This has yet to come up for me in a context where it matters.

The performance hit can be handled by providing a way to enable or disable the extended format, then branching on it.

I think that just printing both sides of the comparison operator would be sufficient for most use cases. This also comes with less of a performance hit.

What about multi-statement expressions in braces? Is this currently allowed in assert? The reference says that they do count as expressions, so I don't see why not. The splitting algorithm here doesn't account for it. While I doubt many users are using it, it should be considered. I could see someone doing it somehow inside a deep macro, however.

@camlorn assert! is always present in all builds. debug_assert! is stripped in release builds.

What performance hit? The RFC only changes what is printed on assertion failure. If your thread is crashing due to a logic error, you hardly care about the number of microseconds it takes to print out the error message.

But as mentioned already, changing what's allowed in assert is not realistically possible. I'd suggest to simplify this to special handling when the expression is a comparison (PartialEq, PartialOrd) of values that implement Debug. Anything more is just a massive waste of time and complexity.

In order to print the expression's intermediate results, the assert macro has to save them all and then drop them all immediately after the assert succeeds. Since it's doing things with them in the failure case, LLVM or similar is not going to be able to eliminate temporaries and optimization passes aren't going to kill the cost.

This is mentioned in the drawbacks section.

I don't agree that's a concern for any real code. If the code's performance is so sensitive, you're going to use debug_assert anyway.

I didn't come up with it, but I do think it's a concern. It might be worth profiling.

My thought is that on X86 current asserts are probably close to free because the branch predictor will just predict correctly save in the case of bugs.

My other concerns are probably more of a problem: there does appear to be a compatibility issue here as well. I could be wrong or misunderstanding the RFC.

I do agree with those other concerns. I also agree with the suggestion to simplify the RFC down to just comparison operators, and stick with current behavior when that doesn't apply. That would solve the major motivation (getting rid of assert_eq), without much code bloat.

What if the special debugging code was present for debug and test builds, but assert had it's present behavior in release builds? (Or have a compiler flag to determine the assert format that is configured to the old style in release styles by default).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. The negated operator may not be true for order operations if the type only implements PartialOrd. For example, f64::NaN < 5 and f64::NaN >= 5 are both false.

We can't fail to compile if intermediate portions of the expression can't be stringified as people are already potentially using assert on things which can be compared but not stringified. This would be a breaking change.

I agree, we should have some fallback if intermediate results don't implement Debug

You would also need a fallback if intermediate results don't implement Copy. Consider this example:

#[derive(Debug, PartialEq)] struct Foo(u32);

impl ::std::ops::Add for Foo { type Output = Self; fn add(self, rhs: Self) -> Self { Foo(self.0 + rhs.0) } }

pub fn main() { let (x, y, z) = (Foo(1), Foo(2), Foo(3)); assert!(x == y + z) }

According to the RFC as written this should show the error message:

thread '<main>' panicked at 'assertion failed:
Expected: x == y + z
With expansion: Foo(1) == Foo(2) + Foo(3)'

but the only way to do so is to prepare that message before executing the expression as y + z consumes both y and z.

Because of this issue I'm also in favor of limiting this to handling PartialEq and PartialOrd; those both force the implementer to operate on references, so you're guaranteed to be able to reuse the operands for printing the error message.

I think we should limit the amount of code included in release builds. For debug and test builds I'm all for having beautiful and informative output. In these (developer build) cases I don't mind the extra costs.

To address the issues of message generation, I propose to make there a default in the case that:

• target doesn't implement Debug
• target is not Copy and using a non-comparison operator

However, I still found boolean-related operators important; if we only did the magic for comparison operators, the message will become useless when you used &&, || or !, which I found it quite common.

Regarding the performance regression, let me explain in detail, using (a() + b()) == c() as a example:

1. Each intermediate value is now explicitly stored as a binding.
2. a() + b() is calculated.
3. While the compiler was able to drop any computation done before, we cannot drop the results of a() and b() now since we may need to show the debug dump.

This takes up extra stack space a/o registers. If the amount of usable registers are reduced, this may slightly degrade the performance. I still support the point that assert is not free, and I think such things are acceptable.

This is cool, but I'd prefer it only for debug_assert!() or for assert!(), but only in debug builds. In release mode I don't care that much what assert prints, but I worry about the overhead.

@pornel The overhead is minimal, as it just takes a little more stack/register space, or only overhead on failure.

I'd prefer it only for debug_assert!()

I think this is most useful in #[test] code, which typically uses assert! not debug_assert!. But I agree that it isn't really necessary for release builds.

aturon added the T-libs-api

Relevant to the library API team, which will review and decide on the RFC.

label

Jun 6, 2017

it just takes a little more stack/register space

Perhaps it can prevent inlining or other optimizations by bloating code.

Ping @dtolnay, can you help drive this to a conclusion?

I am totally on board with better output from the assert macro. This RFC is not introducing any new API or really any irreversible commitment at all, so I don't think it is productive to pin things down any more than is already written.

The things we would be agreeing to here are:

• Macros are nice, let's make the most of them.
• The assert! and debug_assert! macros should recognize some basic common patterns to produce more informative output. For example assert!($expr == $expr) should show the value of the lhs and rhs when it fails.
• The specifics of what patterns to recognize and how to expand them can be settled in PRs.
• Must not break any existing code. This includes code that uses non-Debug types. To me this sounds like it involves specialization which is unstable, but again we are not adding new API here so if worse comes to worse and specialization is scrapped, we are not stuck with an unimplementable API.
• Release builds must not run slower. This may necessitate a different expansion and worse diagnostic for some expressions in release mode.
• Once we have this, assert_eq and assert_ne become totally redundant. Imagine we already had assert!(a == b) produce good output and someone opened an RFC to add assert_eq!(a, b). It would be closed.
• The timeline if and when to deprecate assert_eq and assert_ne can be negotiated later.

@rfcbot fcp merge

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @dtolnay
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

I find the "detailed design" not detailed enough.

1. Will the following be expanded?
   let f = 3.14;
   let g = 2.72;
   assert!(approx_eq(f, g, 0.01));
2. What about if/match/etc expressions used inside the assert?
   assert!(if x > 0 { 1 / x } else { 0 } == 4);
3. Why exclude the .?

@kennytm:

1. No. Functions call are not expanded as of the current description.
2. This is actually unspecified, but IMO it should print if/else details.
3. Because it would be hard to format prettily and it's going to print the same variable twice (once as struct, once as member).

I will update the examples according to the concerns soon. Omitting function call parameters is an unfortunate thing, but I consider it should be formatted with a user provided rule.

@ishitatsuyuki Accessing a field and accessing a method is different though. There is no member in v.is_empty() (if it was a function-typed member you'll need to call it as (v.is_empty)()).

I'm going to check my checkbox, but it's mostly based on what @dtolnay explained above. I find the exact detailed design here, like @kennytm, pretty lacking in how it would happen or also missing crucial details like the point about release builds probably don't want such special treatment.

I think it's a totally fine idea to move assert to a procedural macro that can do "fancier things" so long as we're sure none of those fancier things break existing code.

I'm not really sure the text of the RFC is reflecting the thrust of the FCP, but @ishitatsuyuki would you be interested in updating it along the lines of what @dtolnay commented?

I have updated the text. Feel free to point out when I'm wrong.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's really no reason to include this functionality in release builds at all. You don't want the increased code size, nor this information, in released software.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of Rust's advantage is that you can get backtrace properly even in release builds. Basically, if you don't know how the assertion failed, it would be almost impossible to diagnose the crash.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, and I can appreciate that, but there can also be other constraints in play such as in embedded systems. You might want to disable all the extras and just handle errors that occur by restarting.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add that in commercial software it's also desirable to hide all information about implementation details.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@repax: You could easily redefine your own assert! to hide every detail.

#[cfg(not(debug_assertions))] // <------ macro_rules! assert { ($e:expr) => { if !$e { panic!("assertion failure"); } }; ($e:expr, (((rest:tt)*) => { assert!($e) } }

fn main() { let a = 2; let b = 2; let c = 5; assert!(a + b == c, "note: failed assertion {} + {} == {}", a, b, c); }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kennytm: Why not just make this a compile-time option? Sometimes you want this debugging convenience, often times not. In designing features like this I think it's important to take into account the breadth of contexts in which rust code may operate in the wild.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@repax Define a cargo feature and use #[cfg(feature = "redacted_assert")] then.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My macro probably wouldn't apply to 3rd party crates, so this doesn't quite solve the bloat issue.

We discussed this RFC in the libs team triage today -- and this synopsis in particular. So far we are on board with this RFC but we insist that it must not make assert any slower in release builds.

Please update the RFC to mention that the performance degradation under drawbacks refers to debug builds only.

Also please update the Summary section to provide a better idea of what this RFC proposes. Generic refers to something different. I would mention that we want to implement assert! as a proc macro to recognize some basic common patterns and produce more informative panic messages, for example including the Debug representation of lhs and rhs of comparison operators.

🔔 This is now entering its final comment period, as per the review above. 🔔

The final comment period is now complete.

JP-Ellis added a commit to hep-rs/boltzmann-solver that referenced this pull request

Jun 17, 2018

assert_eq will eventually be deprecated as per RFC 2011 (rust-lang/rfcs#2011).

Signed-off-by: JP-Ellis josh@jpellis.me

JP-Ellis added a commit to hep-rs/boltzmann-solver that referenced this pull request

Jul 15, 2021

assert_eq will eventually be deprecated as per RFC 2011 (rust-lang/rfcs#2011).

Signed-off-by: JP-Ellis josh@jpellis.me

Labels

A-assertions

Proposals relating to assertions.

A-macros-libstd

Proposals that introduce new standard library macros

A-panic

Panics related proposals & ideas

A-test

Proposals relating to testing.

final-comment-period

Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised.

T-libs-api

Relevant to the library API team, which will review and decide on the RFC.