Refactoring/bugfixing around definitions for struct/variant constructors #36814

petrochenkov · 2016-09-29T00:07:13Z

d917c36 separates definitions for struct/variant constructors living in value namespace from struct/variant type definitions.

adfb378 fixes cross-crate resolution of reexports reexporting half-items, like struct constructors without struct type or types without constructor. Such reexports can appear due to glob shadowing.
Resolution now is not affected by the order in which items and reexports are decoded from metadata (cc #31337 (comment)). try_define is not used during building reduced graph anymore.
500 lines of this PR are tests for this exotic situation, the remaining line diff count is actually negative! :)

c695d0c (and partially aabf132) moves most of pattern resolution checks from typeck to resolve (except those checking for associated items), uses the same wording for pattern resolution error messages from both typeck and resolve and makes the messages more precise.

11e3524 fixes seemingly incorrectly set NON_ZERO_SIZED attributes for struct/variant ctors in const eval.

4586fea eliminates ty::VariantKind in favor of def::CtorKind. The logic is that variant kinds are irrelevant for types, they make sense only when we deal with constructor functions/constants. Despite that VariantDefData still keeps a copy of CtorKind, but it's used only for various kinds of pretty-printing (and for storing in metadata).

aabf132 is mostly a cleanup of various impossible or improperly used definitions, and other small definition cleanups.

cc @jseyfried
r? @eddyb

petrochenkov · 2016-09-29T00:09:52Z

src/librustc/hir/def.rs

@@ -123,10 +152,16 @@ impl Def {
            Def::Mod(..) => "module",
            Def::Static(..) => "static",
            Def::Variant(..) => "variant",
+            Def::VariantCtor(.., CtorKind::Fn) => "tuple variant",
+            Def::VariantCtor(.., CtorKind::Const) => "unit variant",
+            Def::VariantCtor(.., CtorKind::Fictive) => "struct variant",


I went with more traditional, but less precise descriptions.
Something like "variant's constructor functions" and "fictive variant constructor" would be more appropriate, but they are a bit long and not very user friendly.

petrochenkov · 2016-09-29T00:11:23Z

src/librustc_mir/transform/deaggregator.rs

@@ -130,7 +130,7 @@ fn get_aggregate_statement_index<'a, 'tcx, 'b>(start: usize,
        debug!("getting variant {:?}", variant);
        debug!("for adt_def {:?}", adt_def);
        let variant_def = &adt_def.variants[variant];
-        if variant_def.kind == VariantKind::Struct {
+        if variant_def.ctor_kind == CtorKind::Fictive {


I'm not sure why this condition is here.
All structs seem to be equivalent at this stage regardless of presence and kind of their constructor.

Yeah this looks like it can only limit the optimization, it should probably be removed. cc @pcwalton

petrochenkov · 2016-09-29T00:13:21Z

src/librustc/hir/def.rs

-    pub name: ast::Name,    // The name of the target.
-    pub def_id: DefId, // The definition of the target.
+    pub name: ast::Name, // The name of the target.
+    pub def: Def, // The definition of the target.


DefId alone is not enough for describing a reexport precisely.
Variants can be exported separately in type and value namespaces, but have a single DefId (and NodeId).

eddyb · 2016-09-29T00:16:44Z

src/librustc/middle/cstore.rs

@@ -122,7 +122,6 @@ pub struct ExternCrate {
 /// can be accessed.
 pub trait CrateStore<'tcx> {
    // item info
-    fn describe_def(&self, def: DefId) -> Option<Def>;


Whoa, how is this even possible?! Even if you managed to remove uses of it, this is important for other things.
E.g. I was telling @solson about describe_def being usable for telling apart static and static mut.
And I've used it in an upcoming PR to replace is_extern_item.

Oh, well. I'll return it back then.
It was completely replaced by Defs in Exports returned by item_children.
describe_def in its removed form has a catch though - variant ctors "decay" into variant types because they share DefId. item_children adjust the children lists to correctly fill both namespaces.

Right, I think the correct thing here is to split the DefId if we need them separate.

petrochenkov · 2016-09-29T00:17:58Z

@nrc, could you look at the changes in save_analysis in aabf132? I'm not sure if it's covered by tests or not.

eddyb · 2016-09-29T00:20:03Z

src/librustc_metadata/decoder.rs

+                            let ctor_kind = self.get_ctor_kind(child_index);
+                            let ctor_def = Def::VariantCtor(def_id, ctor_kind);
+                            callback(def::Export { def: ctor_def, name: name });
+                        }


Am I understanding correctly that Def::Variant is supposed to be solely in the type namespace?
If we really need this distinction, shouldn't they have different DefId's?

If we really need this distinction, shouldn't they have different DefId's?

They should! I suspect it will be a precondition for implementing variant types.
It's a massive refactoring though, a lot of code needs to be adjusted to use new ids.

I actually tried to do this separation the last autumn, a year ago (as a part of #28816), but wasn't able to build even libcore after that.
So, maybe the refactoring wasn't actually so massive and it was just my lack of experience.

eddyb · 2016-09-29T00:28:34Z

cc @nikomatsakis Do you think it's worth to have one DefId for the variant itself and one for the constructor of that variant?

nrc · 2016-09-29T02:23:13Z

src/librustc_save_analysis/dump_visitor.rs

@@ -334,10 +330,14 @@ impl<'l, 'tcx: 'l, 'll, D: Dump + 'll> DumpVisitor<'l, 'tcx, 'll, D> {
                    scope: scope
                }.lower(self.tcx));
            }
+            Def::Local(..) |
+            Def::Upvar(..) |


Why don't you expect to lookup locals here?

The function is used only on exports, so everything non-exportable (locals, associated items, etc) is in the span_bug! list.

nrc · 2016-09-29T02:24:17Z

save-analysis stuff looks OK, module the one question

bors · 2016-10-04T15:37:45Z

☔ The latest upstream changes (presumably #36953) made this pull request unmergeable. Please resolve the merge conflicts.

`try_define` is not used in build_reduced_graph anymore Collection of field names for error reporting is optimized Some comments added

Make error messages more precise

And for methods/functions as well, they are zero-sized now

Address comments + Fix rebase

petrochenkov · 2016-10-04T21:31:50Z

Rebased, updated.

eddyb · 2016-10-04T21:38:09Z

@bors r+

bors · 2016-10-04T21:38:10Z

📌 Commit bc0eabd has been approved by eddyb

bors · 2016-10-04T23:30:31Z

⌛ Testing commit bc0eabd with merge fd065a8...

@jseyfried

Refactoring/bugfixing around definitions for struct/variant constructors d917c36 separates definitions for struct/variant constructors living in value namespace from struct/variant type definitions. adfb378 fixes cross-crate resolution of reexports reexporting half-items, like struct constructors without struct type or types without constructor. Such reexports can appear due to glob shadowing. Resolution now is not affected by the order in which items and reexports are decoded from metadata (cc #31337 (comment)). `try_define` is not used during building reduced graph anymore. 500 lines of this PR are tests for this exotic situation, the remaining line diff count is actually negative! :) c695d0c (and partially aabf132) moves most of pattern resolution checks from typeck to resolve (except those checking for associated items), uses the same wording for pattern resolution error messages from both typeck and resolve and makes the messages more precise. 11e3524 fixes seemingly incorrectly set `NON_ZERO_SIZED` attributes for struct/variant ctors in const eval. 4586fea eliminates `ty::VariantKind` in favor of `def::CtorKind`. The logic is that variant kinds are irrelevant for types, they make sense only when we deal with constructor functions/constants. Despite that `VariantDefData` still keeps a copy of `CtorKind`, but it's used only for various kinds of pretty-printing (and for storing in metadata). aabf132 is mostly a cleanup of various impossible or improperly used definitions, and other small definition cleanups. cc @jseyfried r? @eddyb

bors · 2016-10-05T02:51:07Z

rust-highfive assigned eddyb Sep 29, 2016

petrochenkov commented Sep 29, 2016

View reviewed changes

eddyb reviewed Sep 29, 2016

View reviewed changes

nrc reviewed Sep 29, 2016

View reviewed changes

petrochenkov added 7 commits October 4, 2016 22:20

Separate Def::StructCtor/Def::VariantCtor from Def::Struct/Def::Variant

da7b1c9

Fix cross-crate resolution of half-items created by export shadowing

d19c16a

Further cleanup in resolve

e8ea38e

`try_define` is not used in build_reduced_graph anymore Collection of field names for error reporting is optimized Some comments added

Move pattern resolution checks from typeck to resolve

c95b280

Make error messages more precise

Set NON_ZERO_SIZED flag correctly for struct/union ctors

64bdf1b

And for methods/functions as well, they are zero-sized now

Eliminate ty::VariantKind in favor of def::CtorKind

75d6522

Turn some impossible definitions into ICEs

bd291ce

petrochenkov force-pushed the def branch from a6e3443 to d669a0e Compare October 4, 2016 20:21

Remove some unused methods from metadata

bc0eabd

Address comments + Fix rebase

petrochenkov force-pushed the def branch from d669a0e to bc0eabd Compare October 4, 2016 21:31

bors merged commit bc0eabd into rust-lang:master Oct 5, 2016

This was referenced Oct 5, 2016

Turn compatibility lint match_of_unit_variant_via_paren_dotdot into a hard error #36871

Merged

fix stack overflow by enum and const #36458

Closed

This was referenced Oct 5, 2016

run rustfmt on librustc_typeck/check/method folder #36736

Merged

Refactor match checking to use HAIR #36695

Merged

mcarton mentioned this pull request Oct 6, 2016

Rustup to *rustc 1.14.0-nightly (3210fd5c2 2016-10-05)* and bump to 0.0.95 rust-lang/rust-clippy#1264

Closed

TimNN mentioned this pull request Oct 7, 2016

“Let NullaryConstructor = something;” causes internal compiler error: “tried to overwrite interned AdtDef” #37026

Closed

petrochenkov deleted the def branch March 16, 2017 19:38

petrochenkov mentioned this pull request Apr 26, 2017

Do not show ::{{constructor}} on tuple struct diagnostics #41433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring/bugfixing around definitions for struct/variant constructors #36814

Refactoring/bugfixing around definitions for struct/variant constructors #36814

petrochenkov commented Sep 29, 2016 •

edited

Loading

petrochenkov Sep 29, 2016

petrochenkov Sep 29, 2016

eddyb Sep 29, 2016

petrochenkov Sep 29, 2016

eddyb Sep 29, 2016

petrochenkov Sep 29, 2016 •

edited

Loading

eddyb Sep 29, 2016

petrochenkov commented Sep 29, 2016

eddyb Sep 29, 2016

petrochenkov Sep 29, 2016

petrochenkov Sep 29, 2016

eddyb commented Sep 29, 2016

nrc Sep 29, 2016

petrochenkov Sep 29, 2016

nrc commented Sep 29, 2016

bors commented Oct 4, 2016

petrochenkov commented Oct 4, 2016

eddyb commented Oct 4, 2016

bors commented Oct 4, 2016

bors commented Oct 4, 2016

bors commented Oct 5, 2016

Refactoring/bugfixing around definitions for struct/variant constructors #36814

Refactoring/bugfixing around definitions for struct/variant constructors #36814

Conversation

petrochenkov commented Sep 29, 2016 • 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 Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Sep 29, 2016

bors commented Oct 4, 2016

petrochenkov commented Oct 4, 2016

eddyb commented Oct 4, 2016

bors commented Oct 4, 2016

bors commented Oct 4, 2016

bors commented Oct 5, 2016

petrochenkov commented Sep 29, 2016 •

edited

Loading

petrochenkov Sep 29, 2016 •

edited

Loading