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

Separate `DefId`s for variants and their constructors #59382

Merged
merged 9 commits into from Mar 25, 2019

Conversation

Projects
None yet
4 participants
@davidtwco
Copy link
Member

commented Mar 23, 2019

Part of #44109. Split off from #59376. See Zulip topic for previous discussion.

r? @petrochenkov

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Mar 23, 2019

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

@davidtwco davidtwco force-pushed the davidtwco:rfc-2008-refactoring branch from 9196e73 to 184037a Mar 23, 2019

@petrochenkov petrochenkov force-pushed the davidtwco:rfc-2008-refactoring branch from 184037a to 57f1254 Mar 23, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2019

I reviewed the PR and had comments, but many of them required some experimentation, so I basically started tweaking the code on top of this PR.
I'll hopefully continue and finish making these changes tomorrow.

@petrochenkov petrochenkov changed the title RFC 2008: Refactoring Separated `DefId`s for variants and their constructors Mar 23, 2019

@petrochenkov petrochenkov changed the title Separated `DefId`s for variants and their constructors Separate `DefId`s for variants and their constructors Mar 23, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2019

Any uses of the is_struct/is_tuple/is_unit methods are suspicious (as you could notice, !is_struct was usually used as has_ctor).
(Exhaustive) matching should generally be used instead.
Some code in derive machinery still needs this, but that's because it's very legacy.

Getting from VariantDef to its AdtDef is an uncommon operation, so it's better to remove parent_did from it + the optional DefId introduced some complexity/confusion that wasn't useful in practice.

davidtwco and others added some commits Mar 21, 2019

Separate variant id and variant constructor id.
This commit makes two changes - separating the `NodeId` that identifies
an enum variant from the `NodeId` that identifies the variant's
constructor; and no longer creating a `NodeId` for `Struct`-style enum
variants and structs.

Separation of the variant id and variant constructor id will allow the
rest of RFC 2008 to be implemented by lowering the visibility of the
variant's constructor without lowering the visbility of the variant
itself.

No longer creating a `NodeId` for `Struct`-style enum variants and
structs mostly simplifies logic as previously this `NodeId` wasn't used.
There were various cases where the `NodeId` wouldn't be used unless
there was an unit or tuple struct or enum variant but not all uses of
this `NodeId` had that condition, by removing this `NodeId`, this must
be explicitly dealt with. This change mostly applied cleanly, but there
were one or two cases in name resolution and one case in type check
where the existing logic required a id for `Struct`-style enum variants
and structs.

@petrochenkov petrochenkov force-pushed the davidtwco:rfc-2008-refactoring branch from 57f1254 to 3ca03b3 Mar 24, 2019

@petrochenkov petrochenkov force-pushed the davidtwco:rfc-2008-refactoring branch 2 times, most recently from 95cf143 to 3fe5b1d Mar 24, 2019

@petrochenkov petrochenkov force-pushed the davidtwco:rfc-2008-refactoring branch from 3fe5b1d to 782a6de Mar 24, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

I'll hopefully continue and finish making these changes tomorrow.

Done.

One remaining issue is that Node::Ctor should not contain CtorOf if possible.
CtorOf can be determined by looking at the node's parent in few places where it makes difference.
I tried to do that in petrochenkov@63edbb5, but it turned out that variant ctors and variants themselves are not always connected by the parent-child relation (variant is not a variant ctor's parent), and I didn't investigate how to fix that.

After the Node::Ctor is done, it will be better for CtorOf to live in def.rs together with CtorKind.
(And one more thing that annoys me unproportionally is that in Ctor(hir::CtorOf, DefId, CtorKind) the "secondary" thing (CtorOf) is placed before the "primary" thing (DefId), I'd personally change it to Ctor(DefId, CtorOf, CtorKind) or something.)

davidtwco added some commits Mar 24, 2019

Remove `CtorOf` from `Node::Ctor`.
This commit removes `CtorOf` from `Node::Ctor` as the parent of the
constructor can be determined by looking at the node's parent in the few
places where knowing this is necessary.
Move `CtorOf` into `hir::def`.
This commit moves the definition of `CtorOf` from `rustc::hir` to
`rustc::hir::def` and adds imports wherever it is used.
Re-order fields in `Def::Ctor`.
This commit moves the `DefId` field of `Def::Ctor` to be the first
field.
@davidtwco

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2019

Thanks for all the changes.

One remaining issue is that Node::Ctor should not contain CtorOf if possible.
CtorOf can be determined by looking at the node's parent in few places where it makes difference.
I tried to do that in petrochenkov/rust@63edbb5, but it turned out that variant ctors and variants themselves are not always connected by the parent-child relation (variant is not a variant ctor's parent), and I didn't investigate how to fix that.

I've fixed this. Your patch was pretty much there, but (the misleadingly named) get_parent function was returning a Node::Item as it finds the next parent that is an item, so I used get_parent_node and then it just worked.

After the Node::Ctor is done, it will be better for CtorOf to live in def.rs together with CtorKind.
(And one more thing that annoys me unproportionally is that in Ctor(hir::CtorOf, DefId, CtorKind) the "secondary" thing (CtorOf) is placed before the "primary" thing (DefId), I'd personally change it to Ctor(DefId, CtorOf, CtorKind) or something.)

Fixed both of these.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

Thanks.
3.5 years in the making!
@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

📌 Commit 23cae1d has been approved by petrochenkov

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

@bors p=1
(The changes are somewhat conflict-prone and unblock other work.)

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

⌛️ Testing commit 23cae1d with merge 3752b3d...

bors added a commit that referenced this pull request Mar 24, 2019

Auto merge of #59382 - davidtwco:rfc-2008-refactoring, r=petrochenkov
Separate `DefId`s for variants and their constructors

Part of #44109. Split off from #59376. See [Zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/rfc-2008/near/132663140) for previous discussion.

r? @petrochenkov
@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing 3752b3d to master...

@bors bors added the merged-by-bors label Mar 25, 2019

@bors bors merged commit 23cae1d into rust-lang:master Mar 25, 2019

1 check passed

homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:rfc-2008-refactoring branch Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.