rustc: introduce {ast,hir}::AnonConst to consolidate so-called "embedded constants". #50851

eddyb · 2018-05-18T00:18:53Z

Previously, constants in array lengths and enum variant discriminants were "merely an expression", and had no separate ID for, e.g. type-checking or const-eval, instead reusing the expression's.

That complicated code working with bodies, because such constants were the only special case where the "owner" of the body wasn't the HIR parent, but rather the same node as the body itself.
Also, if the body happened to be a closure, we had no way to allocate a DefId for both the constant and the closure, leading to several bugs (mostly ICEs where type errors were expected).

This PR rectifies the situation by adding another ({ast,hir}::AnonConst) node around every such constant. Also, const generics are expected to rely on the new AnonConst nodes, as well (cc @varkor).

r? @nikomatsakis

petrochenkov · 2018-05-18T01:08:44Z

src/libsyntax/ast.rs

@@ -1857,7 +1866,7 @@ pub struct Variant_ {
    pub attrs: Vec<Attribute>,
    pub data: VariantData,
    /// Explicit discriminant, e.g. `Foo = 1`
-    pub disr_expr: Option<P<Expr>>,
+    pub disr_expr: Option<Const>,


It's a bit strange that const and static initializers are not treated as Const and not visited by fn visit_const, for example, despite being same "constant single-expression bodies".

Is it necessary to introduce Const in AST, btw?
New node ids can be assigned during lowering to HIR too.
At least from syntax/expansion point of view Const is "just an expression".

(I see that some stuff in resolve becomes shorter, but I haven't looked in detail yet.)

Those have items (or trait/impl associated items) they're directly attached to, so they don't need any changes, but also changing them would complicate a bunch of logic that's straight-forward today and would have to jump through an extra hoop.

There's one potential exception to this, which is that static could be considered to be "more disjoint" from its initializer than const and separating them could allow better bookkeeping of the "allocation" vs "value the allocation is initialized with" (cc @oli-obk), but even then it seems a bit stretched.

I wanted to avoid calling the type EmbeddedConst because it's long and potentially even more confusing, but if anyone has a better suggestion, it could use being more explicit.

the static vs its value won't get easier by changing this. Miri's bookkeeping about that is completely sane now.

Is it necessary to introduce Const in AST, btw?
New node ids can be assigned during lowering to HIR too.

Sadly, yes, because the DefId tree is built from the AST, and nodes inside a Const need to be its descendants (this is where some of the ICEs for closures came from).

So, what criterion should a constant Expr satisfy to become Const then? No direct parent with DefId?
Discriminants in enums have a direct parent with DefId to which they are attached - the variant itself, in this sense I don't see how they are different from consts or statics. Is preferring Const just a matter of convenience in this case?
Could all this be documented in a comment for Const?

Also, expressions in range patterns have very similar properties to array sizes, etc (THIS ... AND_THIS), but I guess they don't need a body because you can't smuggle arbitrary expressions into them, paths at most.

The criterion is a requirement of separate const-evaluation (which requires separate type-checking).

Array type lengths and enum discriminants don't (necessarily) have a parent body so they need a separate DefId and body to even be type-checked (array type lengths could have a DefId attached to the NodeId of the TyArray, but enum variants already have a DefId with a different meaning).

The count of an ExprRepeat is technically guaranteed to be nested inside another body, but type-checking the ExprRepeat requires const-evaluating the count, which currently can't be done without the count being a separate body.

In contrast, const and static

Also, expressions in range patterns have very similar properties to array sizes

It might be a good idea to keep those separate too, but type-checking them could require inference from the outer body (because of associated consts), so it's tricky.

I think long-term we should perhaps keep such constant expressions (including ones from paths involving const generics, in the future), in a separate body but type-check them like closures (sharing the inference context of their parent body), while retaining the ability to const-eval them.

Oh. will this make the difference between a variant's discriminant and it's isize value clearer?

will this make the difference between a variant's discriminant and it's isize value clearer?

Doubful.

nikomatsakis · 2018-05-18T19:43:00Z

src/librustc/hir/map/def_collector.rs

@@ -129,30 +113,6 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {

        self.with_parent(def, |this| {
            match i.node {
-                ItemKind::Enum(ref enum_definition, _) => {


huzzah 🎉

nikomatsakis · 2018-05-18T19:48:24Z

src/libsyntax/ast.rs

@@ -921,6 +921,15 @@ pub enum UnsafeSource {
    UserProvided,
 }

+/// A constant (expression) embedded inside a type, an expression,
+/// or used to define the discriminant value of an enum variant.


Maybe we should call this an AnonymousConst or something like that? This would address @petrochenkov's concern about const vs static items to some extent, since they are clearly not anonymous.

Yeah, Const seems too generic for this thing that has a very specific technical purpose rather than represents constant expressions in general.

I was thinking of ty::Const when I picked the name, TBH, but that doesn't make as much sense in the AST/HIR.

I guess AnonConst would work pretty well, thanks!

nikomatsakis · 2018-05-18T19:50:06Z

r=me once we settle this bikeshed

…ed constants.

…ded constants".

eddyb · 2018-05-20T21:18:24Z

@bors r=nikomatsakis

bors · 2018-05-20T21:18:25Z

📌 Commit 26aad25 has been approved by nikomatsakis

bors · 2018-05-20T22:37:13Z

⌛ Testing commit 26aad25 with merge 538fea5...

@varkor

rustc: introduce {ast,hir}::AnonConst to consolidate so-called "embedded constants". Previously, constants in array lengths and enum variant discriminants were "merely an expression", and had no separate ID for, e.g. type-checking or const-eval, instead reusing the expression's. That complicated code working with bodies, because such constants were the only special case where the "owner" of the body wasn't the HIR parent, but rather the same node as the body itself. Also, if the body happened to be a closure, we had no way to allocate a `DefId` for both the constant *and* the closure, leading to *several* bugs (mostly ICEs where type errors were expected). This PR rectifies the situation by adding another (`{ast,hir}::AnonConst`) node around every such constant. Also, const generics are expected to rely on the new `AnonConst` nodes, as well (cc @varkor). * fixes #48838 * fixes #50600 * fixes #50688 * fixes #50689 * obsoletes #50623 r? @nikomatsakis

bors · 2018-05-21T01:23:09Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 538fea5 to master...

And follows rust-lang/rust#50851

rust-highfive assigned nikomatsakis May 18, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2018

petrochenkov reviewed May 18, 2018

View reviewed changes

leoyvens mentioned this pull request May 18, 2018

Closures cannot be constants #50623

Closed

petrochenkov self-assigned this May 18, 2018

nikomatsakis approved these changes May 18, 2018

View reviewed changes

petrochenkov removed their assignment May 19, 2018

rustc: removed unused DefPathData::Initializer DefId's for associat…

072b0f6

…ed constants.

eddyb force-pushed the the-only-constant branch from 38ab622 to ca44bac Compare May 19, 2018 17:31

eddyb changed the title ~~rustc: introduce {ast,hir}::Const to consolidate so-called "embedded constants".~~ rustc: introduce {ast,hir}::AnonConst to consolidate so-called "embedded constants". May 19, 2018

rustc: introduce {ast,hir}::AnonConst to consolidate so-called "embed…

26aad25

…ded constants".

eddyb force-pushed the the-only-constant branch from ca44bac to 26aad25 Compare May 19, 2018 17:35

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 20, 2018

bors merged commit 26aad25 into rust-lang:master May 21, 2018

eddyb deleted the the-only-constant branch May 21, 2018 08:58

mati865 mentioned this pull request May 22, 2018

Build failure with nightly 2018-05-21 rust-lang/rust-clippy#2788

Closed

kngwyu added a commit to kngwyu/racer-nightly that referenced this pull request Jun 1, 2018

Update rustc-ap-syntax to 151.0

ef71bbc

And follows rust-lang/rust#50851

kngwyu mentioned this pull request Jun 1, 2018

Update rustc-ap-syntax to 151.0 racer-rust/racer#861

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: introduce {ast,hir}::AnonConst to consolidate so-called "embedded constants". #50851

rustc: introduce {ast,hir}::AnonConst to consolidate so-called "embedded constants". #50851

eddyb commented May 18, 2018 •

edited

Loading

petrochenkov May 18, 2018

petrochenkov May 18, 2018 •

edited

Loading

eddyb May 18, 2018

oli-obk May 18, 2018

eddyb May 18, 2018 •

edited

Loading

petrochenkov May 18, 2018

petrochenkov May 18, 2018 •

edited

Loading

eddyb May 19, 2018 •

edited

Loading

oli-obk May 19, 2018

eddyb May 19, 2018

nikomatsakis May 18, 2018

nikomatsakis May 18, 2018

petrochenkov May 18, 2018 •

edited

Loading

eddyb May 19, 2018

nikomatsakis commented May 18, 2018

eddyb commented May 20, 2018

bors commented May 20, 2018

bors commented May 20, 2018

bors commented May 21, 2018

rustc: introduce {ast,hir}::AnonConst to consolidate so-called "embedded constants". #50851

rustc: introduce {ast,hir}::AnonConst to consolidate so-called "embedded constants". #50851

Conversation

eddyb commented May 18, 2018 • edited Loading

Choose a reason for hiding this comment

petrochenkov May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov May 18, 2018 • edited Loading

Choose a reason for hiding this comment

eddyb May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 18, 2018

eddyb commented May 20, 2018

bors commented May 20, 2018

bors commented May 20, 2018

bors commented May 21, 2018

eddyb commented May 18, 2018 •

edited

Loading

petrochenkov May 18, 2018 •

edited

Loading

eddyb May 18, 2018 •

edited

Loading

petrochenkov May 18, 2018 •

edited

Loading

eddyb May 19, 2018 •

edited

Loading

petrochenkov May 18, 2018 •

edited

Loading