rust-lang / rust-lang/rust

`assert_eq!` is not 100% hygienic

Open
#131,446 18 comments 17 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-hygiene A-macros C-bug T-compiler T-libs
Dominant language
Rust
Stars
119k
Forks
16.1k
PR merge metrics
PR metrics pending

Description

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the assert_eq! expansion shown in the issue and reproduce the const left_val/right_val example using cargo expand. Investigate Rust macro hygiene and pattern binding behavior, then determine whether the result should address the expansion or improve the diagnostic. Done means the reported collision no longer produces the confusing failure, with coverage for the shown reproducer.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.