Skip to content

derive: clean up hygiene - #32251

Merged
bors merged 3 commits into
rust-lang:masterfrom
durka:derive-2810
Mar 15, 2016
Merged

derive: clean up hygiene#32251
bors merged 3 commits into
rust-lang:masterfrom
durka:derive-2810

Conversation

@durka

@durkadurka commented Mar 14, 2016

Copy link
Copy Markdown
Contributor

derive: clean up hygiene

Fixes#2810.

Spawned from #32139.

r? @alexcrichton

durka added 2 commits March 14, 2016 16:49
@durka
fix FIXME(rust-lang#6449) in #[derive(PartialOrd, Ord)]
4982f91
This replaces some `if`s with `match`es. This was originally not possible
because using a global path in a match statement caused a "non-constant
path in constant expr" ICE. The issue is long since closed, though you still
hit it (as an error now, not an ICE) if you try to generate match patterns
using pat_lit(expr_path). But it works when constructing the patterns more
carefully.
@durka
derive: remove most __ strings FIXME(rust-lang#2810)
fd4fa62
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.
@durka
derive: improve hygiene for type parameters (seerust-lang#2810)
8355389
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).
@alexcrichton

alexcrichton commented Mar 14, 2016

Copy link
Copy Markdown
Member

@bors: r+ 8355389

@bors

bors commented Mar 15, 2016

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 8355389 with merge 3f5db0e...

@alexcrichton

alexcrichton commented Mar 15, 2016

Copy link
Copy Markdown
Member

@bors: retry force clean

@bors

bors commented Mar 15, 2016

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 8355389 with merge 1efa752...

bors added a commit that referenced this pull request Mar 15, 2016
@bors
Auto merge of#32251- durka:derive-2810, r=alexcrichton
1efa752
derive: clean up hygiene
derive: clean up hygiene
Fixes#2810.
Spawned from #32139.
r? @alexcrichton
@bors

bors commented Mar 15, 2016

Copy link
Copy Markdown
Collaborator

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild

@bors
bors merged commit 8355389 into rust-lang:masterMar 15, 2016
This was referenced Mar 15, 2016
Merged
Merged
@Manishearth

Manishearth commented Mar 16, 2016

Copy link
Copy Markdown
Member

This broke warnings:

#![deny(warnings)]#[derive(Hash)]structFoo;fnmain(){}

will now throw a warning/error about an unused argument. Please keep the __s in derive, they're there for a reason.

@durka

durka commented Mar 16, 2016

Copy link
Copy Markdown
ContributorAuthor

Bleh, you're right. The reason was assumed to be hygiene, but it's also
unused arguments/variables. It would be more conventional to have one
underscore, I guess. I can make that change or we can just revert this.

On Wed, Mar 16, 2016 at 2:41 PM, Manish Goregaokar <notifications@github.com

wrote:

This broke warnings:

#![deny(warnings)]
#[derive(Hash)]struct Foo;
fn main() {}

will now throw a warning/error about an unused argument. Please keep the
__s in derive, they're there for a reason.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#32251 (comment)

@Manishearth

Manishearth commented Mar 16, 2016

Copy link
Copy Markdown
Member

Yep. Double underscore is slightly better IMO since clippy uses that to distinguish between things that are silenced because they may be unused (e.g. when they come from a macro), and things which are known to be unused. But single underscore is also ok.

@Manishearth

Manishearth commented Mar 16, 2016

Copy link
Copy Markdown
Member

See #32292

@Manishearth

Manishearth commented Mar 16, 2016

Copy link
Copy Markdown
Member

Should the fix just re-add underscores to the idents introduced in this PR? I'm not entirely sure if that's all that needs to be done.

@durka

durka commented Mar 16, 2016

Copy link
Copy Markdown
ContributorAuthor

Yeah, I should just re-add double underscores I think. The other improvements in this PR are turning an if-else into match (was a FIXME in the code) and making it harder for the derived type parameter to cause a collision, neither of which are critical.

@Manishearth

Manishearth commented Mar 16, 2016

Copy link
Copy Markdown
Member

Cool. I'm working on it.

@durka

durka commented Mar 16, 2016

Copy link
Copy Markdown
ContributorAuthor

Oh okay, well thanks for cleaning up my mess :)

On Wed, Mar 16, 2016 at 2:51 PM, Manish Goregaokar <notifications@github.com

wrote:

Cool. I'm working on it.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#32251 (comment)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@alexcrichtonalexcrichton

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@durka@alexcrichton@bors@Manishearth