Amend #1440: allow const items to contain drop types.

[SergioBenitez]

At present, #1440 does not allow const items to contain drop types. This leads to somewhat surprising behavior, as illustrated in the following example:

#![feature(associated_consts)]
#![feature(const_fn)]
#![feature(drop_types_in_const)]

pub struct Test(Option<Vec<()>>);

impl Test {
    pub const fn new() -> Test {
        Test(None)
    }
}

pub const WORKS: Test = Test(None);

pub const FAILS: Test = Test::new();

fn main() { }

In the code above, WORKS typechecks while FAILS does not, even though they may appear to be identical to the user. The check fails because the compiler treats Test as an opaque type returned from Test::new(); because Test contains a Vec, the type may drop, and so the const item is rejected under the current RFC. This PR changes the semantics to allow the drop type in the const item, thus typechecking FAILS.

[eddyb]

cc @SimonSapin @nikomatsakis

[pnkfelix]

Doesn't the change proposed here explicitly allow for constant expressions that result in destructors being called at runtime?

Or do I misunderstand how the term "constant expression" or "runtime" is being used in this context?

[eddyb]

Oh, right, I prefer the original phrasing because it emphasizes that this is not about code ran at runtime, but rather some compile-time constant evaluation context. I'm not even sure we check this correctly with the feature flag enabled, it's a bit tricky if you can't rely on a full ban of drop types.

[SergioBenitez]

@pnkfelix I was attempting to refer to "compile time evaluation" as "runtime" there, but I can see how that's confusing. I'll switch it to the wording that @eddyb proposes.

[SergioBenitez]

Any further thoughts on this? Would love to get this in and implemented soon!

[nikomatsakis]

Does anyone recall why precisely we prohibited them in the first place? I'm trying to bring that conversation back into cache.

[nikomatsakis]

I'm not sure what this means. My mental model for a constant is that it is an "rvalue" -- in other words, each point where it is used, it is (roughly) "as if" you typed the expression in that place. Therefore, if we permit drop types there, I'd expect that Drop will run at each point where the constant is referenced (and if it is never referenced, it will not run). I think we should expand this text to be much clearer about this point.

[SergioBenitez]

You've precisely identified the intent of the comment. I'll reword it.

[eddyb]

@nikomatsakis It was trying to side-step the "drop runs on every copy which may be unexpected" fact, I believe, and it was quite narrow-scoped, focusing on statics that start out with, e.g. Mutex::new(None) and later on at runtime, own a resource (e.g. a vector), which will never be dropped.
The only reason const fn was included originally was because of constructors of abstractions used for such statics, IIRC, i.e. UnsafeCell::new, Mutex::new (calling the former), eventually Vec::new etc.

[SergioBenitez]

Sorry for the delay. I've reworded to sketchy statement.

[SergioBenitez]

Checking in on this. How's this looking in terms of getting accepted?

[nikomatsakis]

I think that we should be more specific about what "the appropriate point" is. Probably by adding a subsection to allow us to elaborate. But when I started thinking about what to write I encountered a question. I'll post it on the main thread though.

[nikomatsakis]

OK, so, I was thinking more about this RFC. I just wanted to talk through what happens when we have a non-trivial constant. I'm not sure that there's a problem here, but I think the semantics are actually kind of subtle.

Imagine I have this:

const EMPTY_VEC: Vec<i32> = Vec::new();

Here I am assuming Vec::new is a const fn, but you can also imagine that Vec creates such a constant internally, specifying the values for its fields explicitly.

Now, if this were a static, when I reference EMPTY_VEC, I'd have to do it indirectly (i.e., borrow it), because we can't move from a static.

But for a constant, we essentially get a new copy each time we access it. The way I imagine this really working is that we execute Vec::new once (at compile time) and create a template of bytes that we will memcpy for each reference, effectively. Then we will run the destructor. This seems a bit odd, in that we are kind of implicitly saying that Vec is "cloneable" by copying its bytes. This doesn't strike me as true of all types -- in fact, we have a name for such types, which is Copy. The only reason that this is ok for Vec is because we know that for this particular constant value, the result of Drop is a no-op.

Now maybe by declaring Vec::new as a const fn I have kind of declared that I will produce a value whose Drop is idempotent, but it feels a bit implicit to me. Will this always fall out naturally just by limiting the kinds of things that we can do at constant evaluation time?

[nikomatsakis]

@nagisa raises the question of what happens when you use such a constant in a pattern -- though I think the answer there is that we wouldn't really be instantiating the constant in that case, but rather doing a kind of comparison... so maybe drop doesn't have to run? But I'm not sure the best way to think about this. It seems tied in to the annoyance around #[structural_match] as well.

[SimonSapin]

The only reason that this is ok for Vec is because we know that for this particular constant value, the result of Drop is a no-op.

Yes, this is also the case where I’ve wanted this: struct Atom { pub unsafe_data: u64 }; has impls of Clone and Drop that are no-ops for some bit patterns, and increment/decrement a reference count (and deallocate some memory when the count reaches zero) for other bit patterns. Creating a value of the latter kind requires running non-constexpr code, so only the former kind could possibly be in a const item.

Regarding patterns, if fact that was the motivation for using const in the first place. Currently we have a macro that expands to a struct literal which can be used as either an expression or a pattern, but this requires the field to be public (despite the fact that setting it to arbitrary value is unsafe). By having the macro expanding to a path to a const item, we could (hopefully?) make the field private. Since this path-to-const pattern replaces a struct literal, I wouldn’t expect Drop::drop to run for that pattern. (Even though it would be a no-op anyway, in this case.)

Taking another step back, this whole thing would be a work around for privacy hygiene in macro_rules!.

[nikomatsakis]

@eddyb had a nice example of how you could easily misuse this capability:

pub struct FileHandle { x: i32 }
impl Drop for FileHandle { /* closes the handle */ }
const STDIN: FileHandle = FileHandle { x: 0 }; // BAD

Now any reference to STDIN will close file description 0. If you used a static, everything would be fine.

This is not to say we should not permit drop in constants, but there is a kind of subtle "opt in" happening here. It'd be nice if we could do a bit better.

@eddyb suggested perhaps a lint of some kind, but we'd have to tailor it properly.

[nagisa]

I feel that adding something that would then be limited against by default is meaningless. Also nice example.

…

On Feb 22, 2017 13:54, "Niko Matsakis" ***@***.***> wrote: @eddyb <https://github.com/eddyb> had a nice example of how you could easily misuse this capability: pub struct FileHandle { x: i32 }impl Drop for FileHandle { /* closes the handle */ }const STDIN: FileHandle = FileHandle { x: 0 }; // BAD Now any reference to STDIN will close file description 0. If you used a static, everything would be fine. This is not to say we should not permit drop in constants, but there is a kind of subtle "opt in" happening here. It'd be nice if we could do a bit better. @eddyb <https://github.com/eddyb> suggested perhaps a lint of some kind, but we'd have to tailor it properly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1817 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0mFxPTG275Cazxpqwb117FTL29siks5rfCHYgaJpZM4LLp-p> .

[pnkfelix]

doesn't this need further amending to be consistent with the other changes in the RFC?

(This probably ties into @nikomatsakis's earlier point that we need more specifics about what "the appropriate point in the program" is...)

[pnkfelix]

hmm well on further reflection I guess this text may be consistent as written. (Just need to get my head around when the destructors that are now allowed are actually getting invoked.)

[eddyb]

@SergioBenitez @pnkfelix You might want to take a look at rust-lang/rust#40036 where I used a potential problem with this addition as an example. That is, we have to specify in what conditions drops wouldn't run in constant initializers (and thus when values of types with Drop implementations could be allowed).

[nikomatsakis]

If we were going to lint against this, I think the right way would be to say that if a type implements Drop, then a struct constructor that appears in a "constant context" (const/static initializer or body of a const fn) is linted. This means that one can call Vec::new() without a warning, but defining Vec::new() would get a warning unless you silenced it.

@nagisa (I feel like this isn't quite the same as saying we "lint against it by default".)

One could imagine calling the struct body "unsafe" as well.

[pnkfelix]

Summary of status and of comment thread

(Note that this summary includes some discussion of const-evaluation semantics; in particular it includes my personal mental model for the semantics, which probably does not match the rest of the lang/compiler team...)

The RFC proposes to loosen const-expressions so that they can have (sub)expressions of type T where T carries a destructor. (Destructor invocation continues to be disallowed at compile-time.)

The comment thread has raised the question: "Why did const-expressions forbid destructors anyway?" One answer is that people may not expect the resulting semantics with multiple destructor invocations from a single const definition (more details in bulleted summary below).

Niko notes in particular the distinction between static and const here, and provides an example via STDIN where uses of a naive const definition have bad behavior, while an analogous static prevents such bad behavior from arising. There has been a bit of followup discussion about a tailored lint to catch such cases.

---

As I understand it:

• conceptually a const-expression is macro-expanded at the point where its identifier occurs in the program text, and the expression's evaluation, at least in terms of the observable semantics, is delayed until runtime. Thus any destructor invocations are likewise delayed.

  • Update: @eddyb has pointed out macro-expansion may be a poor way to describe the distinction here (in part because it is best not to think of const-evaluation as a source-code transformation). He has proposed the terms "MIR-inlining" versus "MIR-preevaluation and memcpy", and I have revised the remainder of this comment to use the term "MIR-inlining" instead of "macro-expansion."

• Allowing destructors in such a semantics means that a single const definition may lead to zero, one, or many destructor invocations: namely, one for each occurrence of the const identifier that is evaluated during the program execution.

• Note that Niko describes an alternative semantics for const-evaluation (that is probably closer to the actual implementation). Given an idealized set of restrictions on const fn's and const-expressions, I hypothesize that the my MIR-inlining semantics and Niko's memcpy semantics end up equivalent. (I welcome counter examples!)

  I am assuming that in either semantics, invoking any destructor at compile-time signals an error. Note: we do not currently enforce such a rule, which I think is a compiler bug; see Rust Issue 40036. I.e. I want to error at compile-time in a case like this:

  pub static S_FAILS: Option<HasDrop>> = (None, Some(HasDrop)).0;

  (I personally am more ambivalent about failing at compile-time in this related example:

  pub const C_MAY_FAIL: Option<HasDrop>> = (None, Some(HasDrop)).0;

  but that is because I am allowing for a MIR-inlining semantics where every use of C_MAY_FAIL independently constructs and then drops an instance of HasDrop. If we instead dictate the memcpy semantics, where there is no instance of HasDrop at runtime, then I would prefer for this case to fail at compile-time.)

  • Update: Based on discussions with eddyb and arielb1, it sounds like the definition of C_MAY_FAIL should be a compile-time error, along the lines of what I said about the memcpy semantics.

• In any case, under this RFC: the const-evaluator will "continue to" (modulo aforementioned bugs) be responsible for detecting compile-time destructor invocations that have not been delayed to program run time, and signaling an error in such cases. (For example, static items whose initializing expressions invoke destructors, such as in the S_FAILS example above, would be compile-time errors.)

• We could not stabilize such a feature (nor the somewhat misleadingly named drop_types_in_const feature; see Rust Issue 33156) until we have resolved Rust Issue 40036. I'm not sure about whether we would also want to delay implementation until that issue is resolved.

---

Given the caveats above, does the lang team want to take action now on this RFC? Or should we postpone until after resolving Issue 40036?

[eddyb]

macro-expansion semantics

This interacts poorly with associated constants and the future possibility of generic top-level constants.
It's also much more painful to implement and understand, at no real benefit AFAICT.

EDIT: As discussed on IRC, "MIR inlining" is a bit better than "macro-expansion" at describing this.

[pnkfelix]

After reflecting on this RFC, I figure that the capability for abuse is something that the designer of a type can readily defend against.

My basis for this reasoning is: if you don't allow literals to be publicly constructed, and you don't provide a const fn constructor, then your type simply won't run into this issue.

(It might be the case that we will want a lint for any pub const fn whose return type contains Drop things, to force people to opt-in so that they understand the consequences of what they are providing. But I'm not even certain such a lint would pay for itself, and in any case, we can make a decision about whether to add such a lint independently of merging this RFC.)

[pnkfelix]

@rfcbot fcp merge

(see reasoning from my previous comment)

[rfcbot]

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged teams:

• @aturon
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

Concerns:

• unresolved-question-for-lint resolved by Amend #1440: allow const items to contain drop types. #1817 (comment)

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot concern unresolved-question-for-lint

After reflecting on this RFC, I figure that the capability for abuse is something that the designer of a type can readily defend against.

I agree, but my concern is mostly -- will they think to defend against it? This is why I am somewhat reluctant to go forward without a lint of the kind I described (specifically, one the struct literals).

But I've also learned from experience that people hate "allowing" lints -- and this would be a lint that you basically have to allow, to declare that you know what you are doing, if you intend to have types (like Vec) permitted in constants. So my spidey sense tells me that people will hate and not understand this lint, and the concern it is defending against rarely or never arise.

Still, I'd like to ensure we revisit this question before stabilization, so I'm noting a formal concern that we should add an unresolved question about whether to add such a lint.

[aturon]

I've spent some time mulling over this, and want to sketch my mental model of what's going on.

There are types which are not, in general, Copy, but for which certain bitpatterns are effectively Copy (since they can be constructed as a constant). These types may well have destructors. The confusing thing is that, within constant expressions, they look sort of like Copy types but with destructors -- which seems bad indeed.

However, I think there are several mitigating factors that make this perfectly OK:

• This situation can only arise for values of the type with a "copyable bitpattern", and moreover one that is constructable from an API. I expect that in the vast majority of such cases, the destructor is a no-op (what is there to destroy?). That's true for things like Vec<T>. Of course there are counter-examples like owned file descriptors, but that brings me to my next point.

• You usually know if a type has an "interesting" destructor, and I think especially one that does something for copyable bitpatterns of the type. I have a hard time imagining people will get bitten by this.

Finally, if there does turn out to be a footgun here, we always have the option of linting our way out of it.

@rfcbot reviewed

[aturon]

@SergioBenitez, can you add the unresolved question @nikomatsakis mentioned above? I think that's effectively the only remaining blocker here.

[nikomatsakis]

@rfcbot resolve unresolved-question-for-lint

I've decided that this lint doesn't have to be a "formal unresolved question". We can just remember that it is an option if we feel like there is a real footgun evolving here. I tend to agree with @aturon that this probably won't arise in practice all that often.

[aturon]

@nikomatsakis You have to write resolved, with a d (and I can't do it for you).

[cramertj]

Ping @nikomatsakis -- I think your resolved typo above has kept this from going into final comment period for the last 22 days 😆

[aturon]

In the interest in making progress, I've manually applied the FCP tag.

[nikomatsakis]

@rfcbot resolved unresolved-question-for-lint

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period is now complete.

[aturon]

Huzzah! This RFC amendment has been merged! The tracking issue is the one for the original RFC.