Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

propose asserts #1662

Closed
wants to merge 1 commit into from
Closed

Conversation

ashleygwilliams
Copy link
Member

this is an rfc to add several more assert macros. it is a follow up to #1653, specifically @wycats's comment: #1653 (comment).

@nagisa
Copy link
Member

nagisa commented Jun 29, 2016

Why not just

macro_rules! assert_op {
    ($op: tt, $l: expr, $r: expr, $($args: tt)+) => {
        assert!($l $op $r, $($args)+)
    }
}

adds as much additional value as adding a bunch of macros for different operators.

@ticki
Copy link
Contributor

ticki commented Jun 29, 2016

@nagisa What's important here is the formatting.

@durka
Copy link
Contributor

durka commented Jun 29, 2016

Instead of @nagisa's implementation it could be

macro_rules! assert_op {
    ($op:tt, $left:expr, $right:expr) => ({
        match (&$left, &$right) {
            (left_val, right_val) => {
                if !(*left_val $op *right_val) {
                    panic!("assertion failed: `(left {} right)` \
                           (left: `{:?}`, right: `{:?}`)", stringify!($op), left_val, right_val)
                }
            }
        }
    })
}

to get the desired formatting (may need an as_expr hack because of the tt). I don't know if these incremental improvements belong in the standard library, at least right now, because personally I'd prefer a smarter assert that can understand the expression and give much better errors. Since they're just macros, it seems like a space that can be explored by libraries.

@ticki
Copy link
Contributor

ticki commented Jun 29, 2016

@durka I like it.

@Luthaf
Copy link

Luthaf commented Jun 30, 2016

Would an assert_approx_eq! for approximate equality of floats/complex make sense to be in the standard library? I am using this one in my code, and it is really useful for numeric tests.

macro_rules! assert_approx_eq {
    ($left: expr, $right: expr, $tol: expr) => ({
        match (&$left, &$right, $tol) {
            (left_val , right_val, tol_val) => {
                let delta = (left_val - right_val).abs();
                if !(delta < tol_val) {
                    panic!(
                        "assertion failed: `(left ≈ right)` \
                         (left: `{:?}`, right: `{:?}`) \
                         with ∆={:1.1e} (allowed ∆={:e})",
                         *left_val , *right_val, delta, tol_val
                     )
                }
            }
        }
    });
    ($left: expr, $right: expr) => (assert_approx_eq!(($left), ($right), 1e-15))
}

@ashleygwilliams
Copy link
Member Author

ashleygwilliams commented Jun 30, 2016

@Luthaf i think it would! in fact it still exists in the tests for the standard library. https://github.com/rust-lang/rust/search?utf8=%E2%9C%93&q=assert_approx_eq

i have a crate for it here: https://crates.io/crates/assert_approx_eq

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jun 30, 2016
@WaDelma
Copy link

WaDelma commented Jul 6, 2016

I would be vary of adding absolute delta based floating point equality checking.
Better would be include one that works with relative deltas or ULPs.
https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/

@Aatch
Copy link
Contributor

Aatch commented Jul 7, 2016

While I'm not particularly against this, I'm also not sure it's really worth it. It's trivial to define your own macros if you really want inequality ones and I'm not sure asserting inequalities is common enough to warrant special macros for them in the standard library.

@cbreeden
Copy link

I like the idea. I have a few comments, if you don't mind.

  • Should allow for a third optional parameter to describe the purpose of the assert as is now implemented for assert!. See here
  • I personally like the names assert_neq!, assert_leq!, for not equal and less-than or equal but I am very biased from my experience in TeX.

I believe that there is room in the standard library for a decent selection of assertions, and I don't particularly agree with "it's trivial to define your own" so there is no need for this. The code shown in the link for assert! is certainly non-trivial for anyone new the the language.

People who are just starting out may be far more likely to heavily rely on testing libraries while learning, and providing a modest selection of testing capabilities can certainly be very convenient. As opposed to: just write your own macros, or just download a new crate.

@withoutboats
Copy link
Contributor

withoutboats commented Jul 11, 2016

I don't particularly agree with "it's trivial to define your own" so there is no need for this. The code shown in the link for assert! is certainly non-trivial for anyone new the the language.

Big 👍 to this! For the same reason, I think assert_op! is less preferable than 6 macros - the differential for the Rust project in maintaining the extra macros is way smaller than the differential for new users in grokking assert_op!(!=, lhs, rhs) IMO. The idea you can pass the operator as an arg is not at hand for many users, especially since you can only do it in the context of a macro.

If we wanted we could implement them with assert_op! internally (exposing it too, I guess?), that way maintaining them is really only maintaining the one underlying definition.

@brson
Copy link
Contributor

brson commented Jul 18, 2016

I think it makes sense to reduce the surprise factor and add assert_ne and maybe gt, lt.

@BurntSushi
Copy link
Member

I don't have any super strong opinions here personally, although I will note that I'd personally probably never import an entire crate just to get these macros and would instead just write them myself. 'Tis the nature of utility functions and does somewhat bring me on the side of favoring their inclusion in std.

@alexcrichton
Copy link
Member

🔔 This RFC is now entering its week-long final comment period 🔔

The libs team discussed this RFC today (along with #1653) and the overall feeling was that these are a very low maintenance burden, if any, and the road is already paved with assert_eq so it seems reasonable to add more.

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jul 19, 2016
@jonas-schievink
Copy link
Contributor

cc rust-lang/rust#2367, which seems like an alternative to this

@jonas-schievink
Copy link
Contributor

We currently have assert!, assert_eq!, debug_assert! and debug_assert_eq!, but I don't see this RFC mention debug_ variants of the proposed macros. If it doesn't propose to add those, it should at least say so explicitly (and maybe say why).

@jonas-schievink
Copy link
Contributor

If it would add them, it would add 8 macros in total, tripling the number of assert macros so we'd have 12 in total. I count 24 non-assert macros. Seems like a bit much to me :/


Any addition to the standard library will need to be maintained forever, so it is
worth weighing the maintenance cost of this over the value add. Given that it is so
similar to `assert_eq`, I believe the weight of this drawback is low.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's similar, but it won't be used nearly as often. I at least never felt the need to have something like it in std.

@ejmahler
Copy link

ejmahler commented Jul 20, 2016

Rust has taken many steps to reduce roadblocks for unit testing. Cargo integration, attributes, doctests, etc all take away the "it's too much of a pain in the ass to unit test" excuse.

These asserts continue in that direction, so I think it's a good move. I'd also support an "assert fuzzy equality" for floating point numbers, as that would eliminate helper function boilerplate for unit testing floating point math code.

@aturon
Copy link
Member

aturon commented Jul 22, 2016

@BurntSushi

I don't have any super strong opinions here personally, although I will note that I'd personally probably never import an entire crate just to get these macros and would instead just write them myself. 'Tis the nature of utility functions and does somewhat bring me on the side of favoring their inclusion in std.

I agree, and when you put that together with the likely papercut of trying other assertion forms after seeing assert_eq in std, I also lean 👍 on this RFC.

@alexcrichton
Copy link
Member

The libs team got a chance to discuss this RFC during yesterday's triage and the conclusion was to not merge. The data collected by @lambda shows that these may not really be used all that much in practice and with the explicit purpose of just testing the standard library may not be the best place for such macros. In that sense we were a little uncomfortable with the combinatorial explosion of macros and have decided to go with just assert_ne (#1653) for now. We may still have a surprise factor when missing these macros, but we think it'll be much less than missing the assert_ne macro itself.

Thanks of course though for the RFC @ashleygwilliams and the discussion everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.