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

Fix #[derive] for empty tuple structs/variants #35728

Merged
merged 2 commits into from
Aug 30, 2016

Conversation

petrochenkov
Copy link
Contributor

This was missing from #35138

plugin-[breaking-change]
r? @Manishearth

@@ -844,7 +844,7 @@ impl<'a> AstBuilder for ExtCtxt<'a> {
}
fn pat_enum(&self, span: Span, path: ast::Path, subpats: Vec<P<ast::Pat>>) -> P<ast::Pat> {
let pat = if subpats.is_empty() {
PatKind::Path(None, path)
PatKind::Struct(path, Vec::new(), false)
Copy link
Member

Choose a reason for hiding this comment

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

This is a major change cc @mcarton and @dtolnay

PatKind::Struct is now used instead of PatKind::Path for the empty enum case.

Is this 100% necessary, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively I can change interface of the builder and split pat_enum into pat_path/pat_tuple_struct, that would be a more disruptive change.

Copy link
Member

Choose a reason for hiding this comment

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

Serde does not use PatKind so we are not affected.

I don't understand this method. Below, pat_struct produces PatKind::Struct and pat_tuple produces PatKind::Tuple. Why does pat_enum mean PatKind::TupleStruct? Can't enum variants be structs too? Also for the empty case why pick empty PatKind::Struct over empty PatKind::TupleStruct?

Copy link
Contributor Author

@petrochenkov petrochenkov Aug 17, 2016

Choose a reason for hiding this comment

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

Why does pat_enum mean PatKind::TupleStruct? Can't enum variants be structs too?

The name pat_enum is pretty old, from times when braced enum variants Variant{..} were unstable and empty braced structs/variants Variant{} didn't exist, it means either TupleVariant(..) or UnitVariant.

Also for the empty case why pick empty PatKind::Struct over empty PatKind::TupleStruct?

Because it matches anything (see rust-lang/rfcs#1506 for more details).

The alternative I mentioned above (pat_enum -> pat_path/pat_tuple_struct) is much cleaner at cost of larger breakage. I will probably implement it instead.

@Manishearth
Copy link
Member

r=me

@petrochenkov
Copy link
Contributor Author

Updated with AstBuilder::pat_enum refactored (see #35728 (comment) and below).

jseyfried added a commit to jseyfried/rust that referenced this pull request Aug 28, 2016
Fix #[derive] for empty tuple structs/variants

This was missing from rust-lang#35138
bors added a commit that referenced this pull request Aug 30, 2016
Batch up libsyntax breaking changes

Batch of the following syntax-[breaking-change] changes:
 - #35591: Add a field `span: Span` to `ast::Generics`.
 - #35618: Remove variant `Mod` of `ast::PathListItemKind` and refactor the remaining variant `ast::PathListKind::Ident` to a struct `ast::PathListKind_`.
 - #35480: Change uses of `Constness` in the AST to `Spanned<Constness>`.
  - c.f. `MethodSig`, `ItemKind`
 - #35728: Refactor `cx.pat_enum()` into `cx.pat_tuple_struct()` and `cx.pat_path()`.
 - #35850: Generalize the elements of lists in attributes from `MetaItem` to a new type `NestedMetaItem` that can represent a `MetaItem` or a literal.
 - #35917: Remove traits `AttrMetaMethods`, `AttributeMethods`, and `AttrNestedMetaItemMethods`.
  - Besides removing imports of these traits, this won't cause fallout.
 - Add a variant `Union` to `ItemKind` to future proof for `union` (c.f. #36016).
 - Remove inherent methods `attrs` and `fold_attrs` of `Annotatable`.
  - Use methods `attrs` and `map_attrs` of `HasAttrs` instead.

r? @Manishearth
@bors bors merged commit f6e06a8 into rust-lang:master Aug 30, 2016
@petrochenkov petrochenkov deleted the empderive branch September 21, 2016 20:01
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.

4 participants