rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.75k stars 12.65k forks source link

`assert_eq!` is not 100% hygienic #131446

Open GrigorenkoPV opened 1 week ago

GrigorenkoPV commented 1 week ago

Not sure if this should be considered a bug or a diagnostic issue.

Having a const left_val or const right_val declared breaks assert_eq!. This has to do with its expansion and Rust's rules for macro hygiene: https://sabrinajewson.org/blog/truly-hygienic-let

Consider this code

fn main() {
    let x: u8 = 0;
    assert_eq!(x, 0);
}

according to cargo expand it expands to

#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
fn main() {
    let x: u8 = 0;
    match (&x, &0) {
        (left_val, right_val) => {
            if !(*left_val == *right_val) {
                let kind = ::core::panicking::AssertKind::Eq;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    };
}

Since assert_eq! wants to use the value of the provided expressions twice (once for comparison, once for printing the result on failure), but it only wants to evaluate each expression once, it does a match to bind them to a pattern (left_val, right_val). However, having a const named left_val or right_val in scope changes the meaning of the pattern.

fn main() {
    let x: u8 = 0;
    const left_val: i8 = -123;
    assert_eq!(x, 0);
}
error[E0308]: mismatched types
 --> src/main.rs:4:5
  |
3 |     const left_val: i8 = -123;
  |     ------------------ constant defined here
4 |     assert_eq!(x, 0);
  |     ^^^^^^^^^^^^^^^^
  |     |
  |     expected `&u8`, found `i8`
  |     this expression has type `(&u8, &{integer})`
  |     `left_val` is interpreted as a constant, not a new binding
  |     help: introduce a new binding instead: `other_left_val`
  |
  = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0614]: type `i8` cannot be dereferenced
 --> src/main.rs:4:5
  |
4 |     assert_eq!(x, 0);
  |     ^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)

The error message, admittedly, is not very helpful.

Thankfully, you can't use this to make assert_eq pass/fail when it shouldn't. The worst you can achieve is a cryptic error message from the compiler. I think. So this "bug" is not really exploitable, plus chances of accidentally breaking this are probably pretty low (consts are usually named in UPPER_CASE in Rust), but the diagnostic is admittedly not very helpful.

The article I've linked above (https://sabrinajewson.org/blog/truly-hygienic-let) offers a potential solution for this. TL;DR: due to shadowing shenanigans having a function named left_val will prevent left_val from being interpreted as a const in patterns.

@rustbot label A-macros A-diagnostics C-bug D-confusing D-terse

GrigorenkoPV commented 1 week ago

The article I've linked above (https://sabrinajewson.org/blog/truly-hygienic-let) offers a potential solution for this. TL;DR: due to shadowing shenanigans having a function named left_val will prevent left_val from being interpreted as a const in patterns.

Unfortunately if we do this

--- a/library/core/src/macros/mod.rs
+++ b/library/core/src/macros/mod.rs
@@ -41,6 +41,11 @@ macro_rules! panic {
 #[allow_internal_unstable(panic_internals)]
 macro_rules! assert_eq {
     ($left:expr, $right:expr $(,)?) => {
+        {
+        #[expect(dead_code)]
+        fn left_val(){}
+        #[expect(dead_code)]
+        fn right_val(){}
         match (&$left, &$right) {
             (left_val, right_val) => {
                 if !(*left_val == *right_val) {
@@ -52,6 +57,7 @@ macro_rules! assert_eq {
                 }
             }
         }
+        }
     };
     ($left:expr, $right:expr, $($arg:tt)+) => {
         match (&$left, &$right) {

then this starts to compile

let x: () = ();
assert_eq!(x, {left_val()});

so still not 100% hygienic.

GrigorenkoPV commented 1 week ago

I see 3 ways of dealing with this:

  1. Just ignore it.
  2. Tweak the rules around hygiene in macros. Probably a nightmare for backwards compatibility.
  3. Introduce some way to specify that you explicitly want a pattern to not be a const patter. Some special syntax, or a keyword, or an attribute perhaps.
dev-ardi commented 1 week ago

If this hasn't been a problem for the past 9 years we could change the name of left_val and right_val to __left_val and __right_val (like c++ does) to make it even less of a problem

zachs18 commented 1 week ago

We could have assert_eq! (and other macros) use left_val@_ (etc) to give an explicit "match bindings cannot shadow constants" error if someone has a const left_val in scope (the left hand side of a @ pattern is (currently) always an identifer pattern; it is not superseded by path/const patterns like "bare" identifier patterns are).

GrigorenkoPV commented 1 week ago

If this hasn't been a problem for the past 9 years we could change the name of left_val and right_val to __left_val and __right_val (like c++ does) to make it even less of a problem

Well, technically C++ also forbids user-created identifiers to start with _ IIRC, but yeah.

We could have assert_eq! (and other macros) use left_val@_ (etc) to give an explicit "match bindings cannot shadow constants" error if someone has a const left_val in scope (the left hand side of a @ pattern is (currently) always an identifer pattern; it is not superseded by path/const patterns like "bare" identifier patterns are).

This sounds like a good option. Not ideal, but given it is an obscure issue in the first place, probably good enough.

danielhenrymantilla commented 6 days ago
fn feed<Arg, R>(
    arg: Arg,
    f: impl FnOnce(Arg) -> R,
) -> R {
    f(arg)
}

macro_rules! assert_eq {
    (
        $left:expr, $right:expr $(,)?
    ) => (
        $crate::feed(
            (&$left, &$right),
            {
                fn _left_val() {}
                fn _right_val() {}
                |(_left_val, _right_val)| {
                    if !(*_left_val == *_right_val) {
                        // …
                    }
                }
            }
        )
    );
}

😰