[generic_assert] Avoid constant environments

[c410-f3r]

cc #44838

This PR is a work in progress. Requesting an early perf run to evaluate possible impacts.

The generic_assert feature captures variables for printing purposes.

fn foo() {
  let elem = 1i32;
  assert!(&elem);
}


// Expansion
fn foo() {
  let elem = 1i32;
  {
    use ::core::asserting::{TryCaptureGeneric, TryCapturePrintable};
    let mut __capture0 = ::core::asserting::Capture::new();
    let __local_bind0 = &elem;
    if (!&*__local_bind0) {
        (&::core::asserting::Wrapper(__local_bind0)).try_capture(&mut __capture0);
        {
          ::std::rt::panic_fmt(format_args!("Assertion failed: &elem\nWith captures:\n  elem = {0:?}\n", __capture0));
        }
      }
  };
}

The problem is that such a thing is complicated in constant environments. At the current time only strings are allowed and a full support parity with non-constant environments is, as far as I can tell, not feasible in the foreseen future.

fn foo() {
  // !!! ERROR !!!
  const {
    let elem = 1i32;
    assert!(&elem);
  }
}

Therefore, generic_assert will not be triggered in constant environment through an is_in_const_env variable flag originated in the rustc_parse crate.

[rustbot]

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[clubby789]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 01b8e1b with merge a7d860c...

[fmease]

I'm pretty sure this doesn't account for nested non-const items. For example:

// false positive: `assert!(…)` is actually NOT in a const context 
const F: fn() = || { let x = true; assert!(!x); };
fn main() { F() }

// false postive: `assert!(…)` is actually NOT in a const context
const F: fn() = { fn f() { let x = true; assert!(!x); } f };
fn main() { F() }

// false postive: `assert!(…)` is actually NOT in a const context
const F: fn() = { struct S; impl S { fn f() { let x = true; assert!(!x); } } S::f };
fn main() { F() }

// false positive: `assert!(…)` is actually NOT in a const context
const fn f() -> fn() { fn g() { let x = true; assert!(!x); } g }
fn main() { f()() }

// false positive (low priority, not "exploitable" atm): `assert!(…)` is actually NOT in a const context
struct Ty<const N: u8>;
fn main() { _ = Ty::<{ fn _f() { let x = true; assert!(!x) } 0 }>; }

Other issues:

// false negative (you missed `parse_static_item`): `assert!(…)` is actually in a const context
static X: () = { let x = true; assert!(!x) };

// false negative: `assert!(…)` is actually in a const context
#![feature(const_trait_impl)]
#[const_trait]
trait Trait { fn f() { let x = true; assert!(!x); } }

[fmease]

I don't think the parser is the right place for this.

For example, you would never be able to catch this in the parser I believe:

#![feature(const_trait_impl)]
#[cfg_attr(SOME_CFG_SPEC, const_trait)]
trait Trait { fn f() { let x = true; assert!(!x); } }

[fmease]

Does your current approach catch this (it might actually, I'm not sure rn (sth. sth. interpolated AST nodes))?

macro_rules! constify { ($expr:expr) => { const { $expr } } }

fn main() {
    constify! {{ let x = false; assert!(!x); }}
}

[c410-f3r]

Thank you for the review, @fmease.

I also share your concerns about uncovered use-cases as well as possible edge-cases. If somehow it were possible to call something like the is_inside_const_context method defined in HIR things would be much easier.

The feeling of thinking that the parser is probably not the right place is also shared, unfortunately, I am not aware of any other alternative.

So the plan is to cover as many cases as possible (your snippets certainly help in that direction), do a crater run and hope for the best. Without a strategy to avoid constant environments, generic_assert is likely going to remain unstable for a long time.

[fmease]

Swallowed by the force push

@bors try @rust-timer queue

[bors]

⌛ Trying commit b6501c5 with merge 3128760...

[bors]

☀️ Try build successful - checks-actions
Build commit: 3128760 (31287601fc50a566bb7663710d746c26b2fb00e5)

[rust-timer]

Finished benchmarking commit (3128760): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 2.4%, secondary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.4%	[1.7%, 3.4%]	3
Regressions ❌ (secondary)	3.2%	[1.9%, 4.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-4.7%, -2.0%]	2
All ❌✅ (primary)	2.4%	[1.7%, 3.4%]	3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 767.862s -> 767.206s (-0.09%)
Artifact size: 331.04 MiB -> 330.97 MiB (-0.02%)

[c410-f3r]

Thank you @fmease. Looks like the additional size of the Parse structure didn't create a significant performance impact

[c410-f3r]

I will hopefully come back with a reasonable set of tests