Skip to content

/rust

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

deriving cleanup #2810

Closed
catamorphism opened this Issue Jul 5, 2012 · 7 comments

Comments

Assignees
No one assigned
Labels
A-frontend A-syntaxext C-cleanup P-low
Projects
None yet
Milestone
No milestone
8 participants
@catamorphism @sanxiyn @alexcrichton @steveklabnik @jbclements @pnkfelix @durka @rust-highfive
@catamorphism
Copy link
Contributor

catamorphism commented Jul 5, 2012

  • Hygiene
  • Search for "__" strings
  • Don't assume std is the standard library

(from a FIXME in syntax::ext::deriving)
@sanxiyn

This comment has been minimized.

Sign in to view
Copy link
Member

sanxiyn commented Jun 24, 2013

auto_serialize changed to auto_encode which changed to deriving(Encodable). FIXME is still there. I updated the issue accordingly.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Aug 5, 2013

auto_serialize and auto_encode have migrated to deriving(Encodable) and deriving(Decodable).

The third bullet on this list is the same issue as #8242.

The first and second bullets are pretty much the same thing, @jbclements I saw you doing some stuff related to hygiene recently. Do you know if that work will "magically fix" this issue of creating code that can't conflict with user's variable names?
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Aug 12, 2014

Is this issue still relevant? There are still some __ strings, but I'm not sure what should actually be done to fix them.
@jbclements

This comment has been minimized.

Sign in to view
Copy link
Contributor

jbclements commented Sep 2, 2014

I'm guessing these __ identifiers are module-level bindings, right? We don't yet have hygiene for module-level identifiers.
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member

pnkfelix commented Sep 2, 2014

@jbclements ah that is good to know. Am I right in assuming that we want to try to put in hygiene for module level identifiers, and we just have not put in the machinery yet?
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Mar 24, 2015

triage: P-low

I believe most cleanup here has been done and the leftover __ are either implementation details or either aspects of hygiene which haven't been implemented yet, so I no longer think this is a backwards-compatibility problem for libraries.

@rust-highfive rust-highfive added P-low and removed P-backcompat-libs labels Mar 24, 2015

@durka

This comment has been minimized.

Sign in to view
Copy link
Contributor

durka commented Mar 7, 2016

So I went through and looked at the __ strings throughout src/libsyntax_ext/deriving.

They fall into several categories:

  1. function arguments (functions like PartialEq::eq, PartialCmp::partial_cmp, etc)
  2. local bindings, either variables or match pattern captures
  3. type parameters (__H for #[derive(Hash)], __S for #[derive(RustcEncodable)], __D for #[derive(RustcDecodable)])

I believe we can remove the __ from 1 and 2. There is no user code in these functions except for struct and field names, and shadowing those doesn't seem to break things: http://is.gd/5KyXSL expanded (though it does cause non_shorthand_field_patterns warnings)

For the type parameters unfortunately there is a risk of collision: http://is.gd/QYdfx5 expanded
However, it seems to me we could be doing better here: we could actually scan the list of type parameters and make a new one that can't collide.

Thoughts?

durka added a commit to durka/rust that referenced this issue Mar 8, 2016

@durka
derive: remove most __ strings FIXME(rust-lang#2810) 
This changes local variable names in all derives to remove leading
double-underscores. As far as I can tell, this doesn't break anything
because there is no user code in these generated functions except for
struct, field and type parameter names, and this doesn't cause shadowing
of those. But I am still a bit nervous.
658368d

durka added a commit to durka/rust that referenced this issue Mar 8, 2016

@durka
derive: improve hygiene for type parameters (see rust-lang#2810) 
When deriving Hash, RustcEncodable and RustcDecodable, the syntax extension
needs a type parameter to use in the inner method. They used to use __H, __S
and __D respectively. If this conflicts with a type parameter already declared
for the item, bad times result (see the test). There is no hygiene for type
parameters, but this commit introduces a better heuristic by concatenating the
names of all extant type parameters (and prepending __H).
0611aef

@durka durka referenced this issue Mar 8, 2016

Closed

cleanups and fixes for #[derive] #32139

durka added a commit to durka/rust that referenced this issue Mar 9, 2016

@durka
derive: remove most __ strings FIXME(rust-lang#2810) 
This changes local variable names in all derives to remove leading
double-underscores. As far as I can tell, this doesn't break anything
because there is no user code in these generated functions except for
struct, field and type parameter names, and this doesn't cause shadowing
of those. But I am still a bit nervous.
d282ffe

durka added a commit to durka/rust that referenced this issue Mar 9, 2016

@durka
derive: improve hygiene for type parameters (see rust-lang#2810) 
When deriving Hash, RustcEncodable and RustcDecodable, the syntax extension
needs a type parameter to use in the inner method. They used to use __H, __S
and __D respectively. If this conflicts with a type parameter already declared
for the item, bad times result (see the test). There is no hygiene for type
parameters, but this commit introduces a better heuristic by concatenating the
names of all extant type parameters (and prepending __H).
255f833

durka added a commit to durka/rust that referenced this issue Mar 10, 2016

@durka
derive: remove most __ strings FIXME(rust-lang#2810) 
This changes local variable names in all derives to remove leading
double-underscores. As far as I can tell, this doesn't break anything
because there is no user code in these generated functions except for
struct, field and type parameter names, and this doesn't cause shadowing
of those. But I am still a bit nervous.
3c19df6

durka added a commit to durka/rust that referenced this issue Mar 10, 2016

@durka
derive: improve hygiene for type parameters (see rust-lang#2810) 
When deriving Hash, RustcEncodable and RustcDecodable, the syntax extension
needs a type parameter to use in the inner method. They used to use __H, __S
and __D respectively. If this conflicts with a type parameter already declared
for the item, bad times result (see the test). There is no hygiene for type
parameters, but this commit introduces a better heuristic by concatenating the
names of all extant type parameters (and prepending __H).
6e98896

durka added a commit to durka/rust that referenced this issue Mar 10, 2016

@durka
derive: improve hygiene for type parameters (see rust-lang#2810) 
When deriving Hash, RustcEncodable and RustcDecodable, the syntax extension
needs a type parameter to use in the inner method. They used to use __H, __S
and __D respectively. If this conflicts with a type parameter already declared
for the item, bad times result (see the test). There is no hygiene for type
parameters, but this commit introduces a better heuristic by concatenating the
names of all extant type parameters (and prepending __H).
e76a980

durka added a commit to durka/rust that referenced this issue Mar 10, 2016

@durka
derive: remove most __ strings FIXME(rust-lang#2810) 
This changes local variable names in all derives to remove leading
double-underscores. As far as I can tell, this doesn't break anything
because there is no user code in these generated functions except for
struct, field and type parameter names, and this doesn't cause shadowing
of those. But I am still a bit nervous.
fe999e3

durka added a commit to durka/rust that referenced this issue Mar 10, 2016

@durka
derive: improve hygiene for type parameters (see rust-lang#2810) 
When deriving Hash, RustcEncodable and RustcDecodable, the syntax extension
needs a type parameter to use in the inner method. They used to use __H, __S
and __D respectively. If this conflicts with a type parameter already declared
for the item, bad times result (see the test). There is no hygiene for type
parameters, but this commit introduces a better heuristic by concatenating the
names of all extant type parameters (and prepending __H).
c12e906

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 12, 2016

@Manishearth
Rollup merge of rust-lang#32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes rust-lang#2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes rust-lang#21714 (cc @apasel422).
~~Fixes~~ (postponed) rust-lang#24047 (cc @withoutboats @bluss).
Fixes rust-lang#31714 (cc @alexcrichton @bluss).
Fixes rust-lang#31886 (cc @oli-obk).
1fac377

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 12, 2016

@Manishearth
Rollup merge of rust-lang#32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes rust-lang#2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes rust-lang#21714 (cc @apasel422).
~~Fixes~~ (postponed) rust-lang#24047 (cc @withoutboats @bluss).
Fixes rust-lang#31714 (cc @alexcrichton @bluss).
Fixes rust-lang#31886 (cc @oli-obk).
7d30712

bors added a commit that referenced this issue Mar 12, 2016

@bors
Auto merge of #32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).
ea4b981

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 12, 2016

@Manishearth
Rollup merge of rust-lang#32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes rust-lang#2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes rust-lang#21714 (cc @apasel422).
~~Fixes~~ (postponed) rust-lang#24047 (cc @withoutboats @bluss).
Fixes rust-lang#31714 (cc @alexcrichton @bluss).
Fixes rust-lang#31886 (cc @oli-obk).
7ca8fad

bors added a commit that referenced this issue Mar 12, 2016

@bors
Auto merge of #32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).
d8924a4

bors added a commit that referenced this issue Mar 12, 2016

@bors
Auto merge of #32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).
d9b668c

bors added a commit that referenced this issue Mar 12, 2016

@bors
Auto merge of #32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).
8c91b81

bors added a commit that referenced this issue Mar 12, 2016

@bors
Auto merge of #32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).
a5bdac1

durka added a commit to durka/rust that referenced this issue Mar 13, 2016

@durka
derive: remove most __ strings FIXME(rust-lang#2810) 
This changes local variable names in all derives to remove leading
double-underscores. As far as I can tell, this doesn't break anything
because there is no user code in these generated functions except for
struct, field and type parameter names, and this doesn't cause shadowing
of those. But I am still a bit nervous.
b5a0e71

durka added a commit to durka/rust that referenced this issue Mar 13, 2016

@durka
derive: improve hygiene for type parameters (see rust-lang#2810) 
When deriving Hash, RustcEncodable and RustcDecodable, the syntax extension
needs a type parameter to use in the inner method. They used to use __H, __S
and __D respectively. If this conflicts with a type parameter already declared
for the item, bad times result (see the test). There is no hygiene for type
parameters, but this commit introduces a better heuristic by concatenating the
names of all extant type parameters (and prepending __H).
e084a84

bors added a commit that referenced this issue Mar 13, 2016

@bors
Auto merge of #32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).
095af52

bors added a commit that referenced this issue Mar 13, 2016

@bors
Auto merge of #32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).
fb9b646

bors added a commit that referenced this issue Mar 13, 2016

@bors
Auto merge of #32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).
afacbfd

bors added a commit that referenced this issue Mar 14, 2016

@bors
Auto merge of #32139 - durka:derive-fix, r=alexcrichton 
cleanups and fixes for #[derive]

This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit.

- hygiene cleanups
- use `discriminant_value` instead of variant index in `#[derive(Hash)]`
- ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~
- use `intrinsics::unreachable()` instead of `unreachable!()`

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).
97339bc

durka added a commit to durka/rust that referenced this issue Mar 14, 2016

@durka
derive: remove most __ strings FIXME(rust-lang#2810) 
This changes local variable names in all derives to remove leading
double-underscores. As far as I can tell, this doesn't break anything
because there is no user code in these generated functions except for
struct, field and type parameter names, and this doesn't cause shadowing
of those. But I am still a bit nervous.
fd4fa62

durka added a commit to durka/rust that referenced this issue Mar 14, 2016

@durka
derive: improve hygiene for type parameters (see rust-lang#2810) 
When deriving Hash, RustcEncodable and RustcDecodable, the syntax extension
needs a type parameter to use in the inner method. They used to use __H, __S
and __D respectively. If this conflicts with a type parameter already declared
for the item, bad times result (see the test). There is no hygiene for type
parameters, but this commit introduces a better heuristic by concatenating the
names of all extant type parameters (and prepending __H).
c59400c

@durka durka referenced this issue Mar 14, 2016

Merged

derive: clean up hygiene #32251

durka added a commit to durka/rust that referenced this issue Mar 14, 2016

@durka
derive: improve hygiene for type parameters (see rust-lang#2810) 
When deriving Hash, RustcEncodable and RustcDecodable, the syntax extension
needs a type parameter to use in the inner method. They used to use __H, __S
and __D respectively. If this conflicts with a type parameter already declared
for the item, bad times result (see the test). There is no hygiene for type
parameters, but this commit introduces a better heuristic by concatenating the
names of all extant type parameters (and prepending __H).
8355389

bors added a commit that referenced this issue Mar 15, 2016

@bors
Auto merge of #32251 - durka:derive-2810, r=alexcrichton 
derive: clean up hygiene

derive: clean up hygiene

Fixes #2810.

Spawned from #32139.

r? @alexcrichton
3f5db0e

bors added a commit that referenced this issue Mar 15, 2016

@bors
Auto merge of #32251 - durka:derive-2810, r=alexcrichton 
derive: clean up hygiene

derive: clean up hygiene

Fixes #2810.

Spawned from #32139.

r? @alexcrichton
1efa752

@bors bors closed this in #32251 Mar 15, 2016

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.