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

Warn when using [foo; 0] where foo is a non-const expression that evaluates to a Drop type #79580

Open
bstrie opened this issue Nov 30, 2020 · 5 comments
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-destructors Area: destructors (Drop, ..) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-zst Area: Zero-sized types (ZST). T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@bstrie
Copy link
Contributor

bstrie commented Nov 30, 2020

In #74836 it was discovered that [foo; 0] will leak foo, causing observable effects if foo has a destructor. In response #74857 was opened in order to warn users against using [foo; 0] to construct a zero-length array in favor of [], but this was deemed to be overkill.

However, since then some new information has emerged that I believe calls for re-evaluating this decision. The problem comes from the use of array-repeat expressions in constant contexts. Consider the following program, which works on stable Rust today:

struct Foo;

impl Drop for Foo {
    fn drop(&mut self) {
        println!("dropped!")
    }
}

const FOO: Foo = Foo;

fn main() {
    println!("0 drops follow this");
    [FOO; 0];
    println!("1 drops follow this");
    [FOO; 1];
    println!("2 drops follow this");
    [FOO; 2];
}

While the stabilization of array-repeat expressions on constant values was accidental (#79270), people appear to be in agreement that this behavior is correct: #79270 (comment) . Specifically, it is considered correct that a constant array repeat expression [FOO; N] will run FOO's destructor N times.

Consider how this compares to the use of array-repeat expressions in non-constant contexts. For some non-constant expression [foo; N] where foo impls Drop:

  • When N is greater than 2, this will produce an insurmountable compile-time error (the compiler requires Copy, which is incompatible with Drop).

  • When N is 1, this will have identical results to the const-context array-repeat expression, and is not a problem.

  • When N is 0, this will run the destructor exactly once, which differs from the behavior of const-context array-repeat expressions, which will not run any destructor. (Technically this is contingent on Initializing a zero-length array leaks the initializer #74836 being fixed, but everybody agrees that the current leaking behavior is incorrect).

In light of this divergence of behavior between const and non-const array-repeat expressions, it's worth discussing whether #74857 should be revived, especially since the fix is as simple as not using [foo; 0] and simply using [] instead.

@bstrie bstrie added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Nov 30, 2020
@jyn514 jyn514 added A-const-eval Area: constant evaluation (mir interpretation) A-destructors Area: destructors (Drop, ..) T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 30, 2020
@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2020

Specifically, it is considered correct that a constant array repeat expression [FOO; N] will run FOO's destructor N times.

FWIW, so far I think I am the only one who explicitly said that this is the desired behavior. I would not treat this as consensus yet, since the lang team has not made a statement on the subject.

When N is greater than 2, this will produce an insurmountable compile-time error (the compiler requires Copy, which is incompatible with Drop).

This is only part of the story. The compiler will then try to use promotion to turn this into an array repeat expression of a constant. However, promotion ensures that there is no destructor that needs running, so the conclusion remains the same.


On the subject of the lint itself, I have no strong opinion. However, I do feel rather strongly that #74836 should be fixed by dropping the affected value. We should first get our semantics right, then we can consider if we want to lint against [expr; 0]. Once we did that I feel like the motivation for the lint is rather weak -- I have a hard time imagining how this would actually be a problem in practice.

@bstrie
Copy link
Contributor Author

bstrie commented Dec 1, 2020

I would not treat this as consensus yet, since the lang team has not made a statement on the subject.

The lang team has just agreed to merge #79270, so as of today we can confidently call this the intended behavior.

However, promotion ensures that there is no destructor that needs running, so the conclusion remains the same.

I admit that I'm a bit uncertain regarding promotion rules; I see there's a document at https://github.com/rust-lang/const-eval/blob/master/promotion.md , but shouldn't something this semantically important have its own associated RFC and tracking issue?

I do feel rather strongly that #74836 should be fixed by dropping the affected value

Indeed, I don't think anyone's in disagreement about this. Non-const [foo; 0] will run one destructor, and const [foo; 0] will run zero. It's the existence of this curious edge case that is sole motivation for this issue; notably, the goal is not to stop people from using zero-size arrays, but rather to encourage them to initialize them the idiomatic way via [].

@RalfJung
Copy link
Member

RalfJung commented Dec 2, 2020

The lang team has just agreed to merge #79270, so as of today we can confidently call this the intended behavior.

The lang team has made no statement about the ;0 case there. I have no idea where you are getting your confidence from.

I admit that I'm a bit uncertain regarding promotion rules; I see there's a document at https://github.com/rust-lang/const-eval/blob/master/promotion.md , but shouldn't something this semantically important have its own associated RFC and tracking issue?

Promotion came to be with an RFC (well, back then implementation usually predated RFCs, but the RFC kind of describes the situation at the time, from all I know) and since then unfortunately was extended in an ad-hoc fashion without RFCs or really any kind of design. For more than a year now we are trying to document the status quo, and still we find new ways in which reality differs from the above document. For example, turns out what I said about drops above is wrong; that only applies to lifetime extension, not array repeat expressions. (But possibly it should.)

So yes, it should have its own associated RFC and whatnot. But sadly it doesn't, and I spent a lot of this year cleaning that up. We're getting somewhere though!

@workingjubilee workingjubilee added the A-zst Area: Zero-sized types (ZST). label Oct 6, 2021
@JakobDegen
Copy link
Contributor

JakobDegen commented Aug 31, 2022

Could someone close this? The underlying bug with [foo; 0] not dropping has been fixed nevermind, I misunderstood the issue. This isn't asking what I thought it was

@RalfJung
Copy link
Member

@JakobDegen sure, but we should leave a link to the PR that fixed it -- do you have a PR number?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-destructors Area: destructors (Drop, ..) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-zst Area: Zero-sized types (ZST). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants