Closures cannot be constants #50623

leoyvens · 2018-05-10T18:48:23Z

During type collection, error if a closures is found in constant position, catching that before they go causing ICEs.

During type collection, error if a closures is found in constant position, catching that before they go causing ICEs. Fixes rust-lang#50600. Fixes rust-lang#48838.

rust-highfive · 2018-05-10T18:48:26Z

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

cramertj · 2018-05-10T21:11:57Z

src/librustc_typeck/collect.rs

            if gen.is_some() {
                let hir_id = tcx.hir.node_to_hir_id(node_id);
                return tcx.typeck_tables_of(def_id).node_id_to_type(hir_id);
            }

+            if !tcx.has_typeck_tables(def_id) {
+                span_err!(tcx.sess, span, E0912, "closures cannot be constants");


Nit: closures can be constants:

const FOO: fn() = || println!("Gah!");

True, what about expected numerical constant, found closure?

That sounds much better to me, thanks!

Do we need this check at all? I'd assume if we left it out, we'd get the real typeck error.

petrochenkov · 2018-05-10T23:33:48Z

r? @oli-obk

leoyvens · 2018-05-11T14:43:15Z

I changed the error message.

@oli-obk We need the check because these closures have no outer environment and therefore no typeck tables, so type check ICEs in tcx.typeck_tables_of right here.

oli-obk · 2018-05-11T15:04:57Z

@leodasvacas I understand that we need the check in the const interpreter, but typeck can already correctly process type mismatches in e.g. statics.

static U: usize = |x: u8| {};

error[E0308]: mismatched types
 --> src/main.rs:1:19
  |
1 | static U: usize = |x: u8| {};
  |                   ^^^^^^^^^^ expected usize, found closure
  |
  = note: expected type `usize`
             found type `[closure@src/main.rs:1:19: 1:29]`

so... we should probably figure out how to make the parent of the closure be the constant?

oli-obk · 2018-05-11T15:09:28Z

The PR as it stands probably errors on

struct Foo ([u8; (|x: u8| { }, 9).1]);
enum Functions {
    Square = (|x:i32| { }, 42).1,
}

And I think that worked at some point in time.

leoyvens · 2018-05-11T20:51:52Z

Good examples, this PR indeed errors on them but the current stable will ICE on them, don't know if it worked on older versions.

we should probably figure out how to make the parent of the closure be the constant?

That could work, but what do you mean by "the constant"? I tried attaching closure and other sub-exprs to outermost expr of a Repeat expr ~~but it didn't work, don't know why~~ I was misunderstanding what at Repeat is, will try some more. Also I noticed that struct Foo ([u8; { |x: u8| { } } ]); (with the block around the closure) will currently give an ordinary typeck error rather than ICE (don't know why either) but with this patch will give the new error.

leoyvens · 2018-05-12T15:36:39Z

While playing with this I found another issue #50688, not sure if related.

oli-obk · 2018-05-12T15:41:20Z

Sounds very much related (taking the wrong surrounding scope (main instead of the constant)).

oli-obk · 2018-05-12T15:46:40Z

don't know if it worked on older versions.

1.10 works: https://godbolt.org/g/UJoqTK
1.15 says "unsupported const expr": https://godbolt.org/g/SxsEPn
1.22 and above ices

…r of some const exprs

leoyvens · 2018-05-12T16:13:57Z

In the previous commit I made some progress fiddling with the definition tree, now the struct example works. I opened a bug to track the enum discriminant example, there is probably a better structural fix to all these issues but this PR already makes good progress.

oli-obk · 2018-05-12T16:15:17Z

src/librustc/hir/map/def_collector.rs

@@ -268,34 +269,41 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
    fn visit_expr(&mut self, expr: &'a Expr) {
        let parent_def = self.parent_def;

-        match expr.node {
+        self.parent_def = match expr.node {


can you store this in a temporary variable and assign the self.parent_def = temp.or(self.parent_def) below? I think it would make the steps involved here a bit clearer

oli-obk · 2018-05-12T16:17:14Z

src/test/run-pass/ctfe/closure-in-constant.rs

+struct Foo ([u8; (|x: u8| { }, 9).1]);
+
+// FIXME(#50689) We'd like the below to also work,
+// but due to how DefCollector handles discr_expr differently it doesn't right now.


can you also add a this fixme the the location where discr_expr is handled?

leoyvens · 2018-05-12T17:23:03Z

Thank you for the review, I've addressed the comments.

oli-obk · 2018-05-14T20:49:03Z

src/librustc_typeck/collect.rs

            if gen.is_some() {
                let hir_id = tcx.hir.node_to_hir_id(node_id);
                return tcx.typeck_tables_of(def_id).node_id_to_type(hir_id);
            }

+            if !tcx.has_typeck_tables(def_id) {


Do we still need this check? I thought the DefIds were setup correctly now?

The def ids in discriminants will still have the enum as parent (so will fail here), for structs a case like struct Foo([u8; |x: u8| { }]) will also fail because the closure itself is the outer expression so it will have no parent to attach. So yhea without the check this would still ICE in some cases.

oh... I thought the changes that made (|x: u8| {}, 42).1 work also got rid of the ICE.

oli-obk · 2018-05-15T08:17:36Z

@bors r+

bors · 2018-05-15T08:17:37Z

📌 Commit 0356813 has been approved by oli-obk

nikomatsakis · 2018-05-17T15:19:31Z

@bors r-

Wait, hold up. We're supposed to — at least in some cases — permit closures as constants.

cc @eddyb

leoyvens · 2018-05-18T01:55:24Z

Closing in favour of #50851 which is a real fix.

@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

closures cannot be constants

7bb6de0

During type collection, error if a closures is found in constant position, catching that before they go causing ICEs. Fixes rust-lang#50600. Fixes rust-lang#48838.

rust-highfive assigned petrochenkov May 10, 2018

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

cramertj reviewed May 10, 2018

View reviewed changes

rust-highfive assigned oli-obk and unassigned petrochenkov May 10, 2018

Change error message for closures in constants

de03835

leoyvens mentioned this pull request May 12, 2018

Closures in enum discriminants error when they shouldn't #50689

Closed

In def collection attach closure and other sub-exprs to outermost exp…

9c12e32

…r of some const exprs

oli-obk reviewed May 12, 2018

View reviewed changes

Address review

0356813

oli-obk reviewed May 14, 2018

View reviewed changes

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 15, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 17, 2018

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2018

eddyb mentioned this pull request May 18, 2018

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

Merged

leoyvens closed this May 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closures cannot be constants #50623

Closures cannot be constants #50623

leoyvens commented May 10, 2018

rust-highfive commented May 10, 2018

cramertj May 10, 2018 •

edited

Loading

leoyvens May 10, 2018

cramertj May 10, 2018

oli-obk May 11, 2018

petrochenkov commented May 10, 2018

leoyvens commented May 11, 2018

oli-obk commented May 11, 2018

oli-obk commented May 11, 2018

leoyvens commented May 11, 2018 •

edited

Loading

leoyvens commented May 12, 2018

oli-obk commented May 12, 2018

oli-obk commented May 12, 2018 •

edited

Loading

leoyvens commented May 12, 2018

oli-obk May 12, 2018

oli-obk May 12, 2018

leoyvens commented May 12, 2018

oli-obk May 14, 2018

leoyvens May 14, 2018

oli-obk May 15, 2018

oli-obk commented May 15, 2018

bors commented May 15, 2018

nikomatsakis commented May 17, 2018

leoyvens commented May 18, 2018

Closures cannot be constants #50623

Closures cannot be constants #50623

Conversation

leoyvens commented May 10, 2018

rust-highfive commented May 10, 2018

cramertj May 10, 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

petrochenkov commented May 10, 2018

leoyvens commented May 11, 2018

oli-obk commented May 11, 2018

oli-obk commented May 11, 2018

leoyvens commented May 11, 2018 • edited Loading

leoyvens commented May 12, 2018

oli-obk commented May 12, 2018

oli-obk commented May 12, 2018 • edited Loading

leoyvens commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoyvens commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented May 15, 2018

bors commented May 15, 2018

nikomatsakis commented May 17, 2018

leoyvens commented May 18, 2018

cramertj May 10, 2018 •

edited

Loading

leoyvens commented May 11, 2018 •

edited

Loading

oli-obk commented May 12, 2018 •

edited

Loading