rust-lang / rust-lang/rust

Possible disagreement on categorisation of value during MIR build and PromoteTemps pass

Open
#147,021 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-array A-const-eval A-MIR C-bug I-lang-radar T-compiler
Dominant language
Rust
Stars
119k
Forks
16.2k
PR merge metrics
PR metrics pending

Description

I tried this code, before #145277, courtesy of @traviscross in a comment:

use core::{mem, ptr, sync::atomic::{AtomicU64, Ordering}};

static COUNT: AtomicU64 = AtomicU64::new(0);

struct S;
impl Drop for S {
    fn drop(&mut self) {
        COUNT.fetch_add(1, Ordering::Relaxed);
    }
}
trait Tr {}
impl Tr for S {}

const X: Box<dyn Tr> = unsafe {
    mem::transmute::<_, Box<S>>(ptr::dangling_mut::<S>())
};

const fn x() -> Box<dyn Tr> { X }

fn main() {
    assert_eq!(0, COUNT.load(Ordering::Relaxed));
    let _: [Box<dyn Tr>; 0] = [X as Box<dyn Tr>; 0]; //~ Drops.
    assert_eq!(1, COUNT.load(Ordering::Relaxed));
    let _ = [X as Box<dyn Tr>; 0]; //~ Drops.
    assert_eq!(2, COUNT.load(Ordering::Relaxed));
    let _: [Box<dyn Tr>; 0] = [X; 0]; //~ Drops.
    assert_eq!(3, COUNT.load(Ordering::Relaxed));
    let _: [Box<dyn Tr>; 0] = [x(); 0]; //~ Drops.
    assert_eq!(4, COUNT.load(Ordering::Relaxed));
    let _ = [x(); 0]; //~ Drops.
    assert_eq!(5, COUNT.load(Ordering::Relaxed));
    let _ = [const { x() }; 0]; //~ Doesn't drop.
    assert_eq!(5, COUNT.load(Ordering::Relaxed));
    let _ = [const { x() as Box<dyn Tr> }; 0]; //~ Doesn't drop.
    assert_eq!(5, COUNT.load(Ordering::Relaxed));
    let _ = [const { x() } as Box<dyn Tr>; 0]; //~ Drops.
    assert_eq!(6, COUNT.load(Ordering::Relaxed));
}

I expected to see this happen: every assert_eq! should trigger

Instead, this happened: execution passes

The reason I am opening this issue is to discuss a comprehensive plan to resolve the remaining concern over #145277. We would discover more corner cases and it would become unmanagable if we have to fix these cases one at a time.

Problem statement

Current MIR building from THIR, when repeat expression [$expr; $const] is concerned, needs to know the following information to decide how this kind of expression would be lowered:

  • whether the constant $const takes a value of 0, 1 or anything else
  • whether $expr has a Copy type
  • whether $expr is a const-promotable
    The prominent feature of the lowering is whether $expr shall be "built" for evaluation. If $expr is materialised, it has significance such that a drop is mandatory. However, the materialisation can be superfluous if the value would not end up used in the repeat expression because $const could be statically evaluated to 0; or the drop is unnecessary because $const could be statically evaluated to 1.
type of $expr \ compile-time value of $const 0 1 any
.. is const-promotable or implements Copy should not materialise materialise once materialise multiple times
otherwise materialise and drop materialise exactly once reject

However, determine whether $expr is const-promotable happens after the MIR building. The MIR builder has to apply a simple, but not satisfyingly comprehensive, check to decide which case in the table above shall be applicable.

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 by reproducing the provided Rust example and reading the MIR-building-from-THIR discussion around repeat expressions and the PromoteTemps pass, including #145277. Compare the observed drops with the table in the issue; done means agreeing on a comprehensive lowering plan and covering the identified corner cases with regression tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.