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

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

Merged
merged 8 commits into from
Oct 5, 2016

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Sep 29, 2016

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

@@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@petrochenkov
Copy link
Contributor Author

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

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 });
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

eddyb commented Sep 29, 2016

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

@@ -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(..) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you expect to lookup locals here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nrc
Copy link
Member

nrc commented Sep 29, 2016

save-analysis stuff looks OK, module the one question

@bors
Copy link
Contributor

bors commented Oct 4, 2016

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

Address comments + Fix rebase
@petrochenkov
Copy link
Contributor Author

Rebased, updated.

@eddyb
Copy link
Member

eddyb commented Oct 4, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2016

📌 Commit bc0eabd has been approved by eddyb

@bors
Copy link
Contributor

bors commented Oct 4, 2016

⌛ Testing commit bc0eabd with merge fd065a8...

bors added a commit that referenced this pull request Oct 4, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants