Provide better names for builtin deriving-generated attributes #49986

zofrex · Apr 15, 2018

First attempt at fixing #49967

Not in love with any choices here, don't be shy if you aren't happy with anything :)

I've tested that this produces nicer names in documentation, and that it no longer has issues conflicting with constants with the same name. (I guess we could make a test for that... unsure if that would be valuable)

In all cases I took the names from the methods as declared in the relevant trait.

In some cases I had to prepend the names with _ otherwise there were errors about un-used variables. I'm uneasy with the inconsistency... do they all need to be like that? Is there a way to generate an alternate impl or use a different name (_?) in the cases where the arguments are not used?

Lastly the gensym addition to Ident I implemented largely as suggested, but I want to point out it's a little circuitous (at least, as far as I understand it). cx.ident_of(name) is just Ident::from_str, so we create an Ident then another Ident from it. Ident::with_empty_ctxt(Symbol::gensym(string)) may or may not be equivalent, I don't know if it's important to intern it then gensym it. It seems like either we could use that, or if we do want a new method to make this convenient, it could be on Ident instead (from_str_gensymed?)

pietroalbini · Apr 16, 2018

Thanks for the PR! Highfive failed to assign a reviewer for you, so I'm picking one randomly from the compiler team.

r? @petrochenkov

zofrex · Apr 16, 2018

Manish was mentoring me on this one so I think it makes sense for them to be the one to review this?

r? @Manishearth

Manishearth · Apr 16, 2018

Manishearth approved these changes Apr 16, 2018

View changes

Manishearth · Apr 16, 2018

src/libsyntax_ext/deriving/debug.rs

@@ -40,7 +40,7 @@ pub fn expand_deriving_debug(cx: &mut ExtCtxt,
                          name: "fmt",
                          generics: LifetimeBounds::empty(),
                          explicit_self: borrowed_explicit_self(),
-                          args: vec![fmtr],
+                          args: vec![(fmtr, "_f")],


probs should be fmt

(check what the trait uses)

The trait uses "f" (I took all the names from the traits)

Ah. Probably should be fmt then

Oh, I know why it's short; it's to encourage implementors to keep it short. Leave it f then.

Manishearth · Apr 16, 2018

src/libsyntax_ext/deriving/decodable.rs

-                          args: vec![Ptr(Box::new(Literal(Path::new_local(typaram))),
-                                         Borrowed(None, Mutability::Mutable))],
+                          args: vec![(Ptr(Box::new(Literal(Path::new_local(typaram))),
+                                         Borrowed(None, Mutability::Mutable)), "d")],


Also from the trait. Relatedly... are these (decodable/encodable) even used? I couldn't find any references to these in the docs.

They are, they're from a deprecated builtin thing. See the rustc_serialize crate.

Manishearth · Apr 16, 2018

src/libsyntax_ext/deriving/encodable.rs

-                args: vec![Ptr(Box::new(Literal(Path::new_local(typaram))),
-                           Borrowed(None, Mutability::Mutable))],
+                args: vec![(Ptr(Box::new(Literal(Path::new_local(typaram))),
+                           Borrowed(None, Mutability::Mutable)), "s")],


Manishearth · Apr 16, 2018

Looking good so far. Get rid of the underscores and cherry pick d5a304e instead, that should fix the unused variables warning.

Manishearth · Apr 16, 2018

If you want, can you also try and replace the underscores with hygenic thingies for __cmp and __self_*? Basically, there should be no trace of __ left in src/libsyntax_ext/deriving/. This can also be done in a separate PR if you'd like, but it would be nicer to have it all in one place.

Also, we should have a test ensuring the following compiles:

pub const __arg_0: u8 = 1;

#[derive(PartialEq)]
struct Foo;

zofrex · Apr 16, 2018

I'll make all the suggested changes. Let me know what you think regarding the names that are in doubt. :)

Manishearth · Apr 16, 2018

Yeah just keep the names as is, mirroring the traits.

zofrex · Apr 19, 2018

I take it __cmp should be replaced with cmp but what the __self args should be named is less obvious to me. Here's an example of some generated code:

    fn eq(&self, other: &Foobar) -> bool {
        match *other {
            Foobar { value: ref __self_1_0, value2: ref __self_1_1 } =>
            match *self {
                Foobar { value: ref __self_0_0, value2: ref __self_0_1 } =>
                true && (*__self_0_0) == (*__self_1_0) &&
                    (*__self_0_1) == (*__self_1_1),
            },
        }
    }

I'm not really sure why these are all named self in the first place. How about other_0 and other_1 instead of __self_1_0 and self_1_1, and self_0 and self_1 instead of self_0_0 and self_0_1?

zofrex · Apr 19, 2018

There's a __D in decodable and __S in encodable and __H in hash that all make it into generated code. They're all hygienic (if that's the right term? Nothing explodes if I make a const named __D or __S or __H) but perhaps they could have better names. Thoughts?

Manishearth · Apr 19, 2018

Yeah ideally they should tack on to the argument name. They're called self because the type is Self, but i don't like that much. If you want to tackle that go ahead ( but it's acceptable to land this with just the arguments and lint fixed)

Manishearth · Apr 19, 2018

Also, can you add some tests that ensure consts don't clash?

zofrex · Apr 19, 2018

There are two instances of __arg<i> in format.rs (one, two) that I'm not sure whether they need dealing with or not. If those need fixing up too, I need help figuring out where those get used (first off: what names do they end up with, and when does this code come into play, because I have not succeeded at creating a failing case. I'll post more details if you do want those fixed too)

Manishearth · Apr 19, 2018

Yeah, we want to ideally fix all underscores and make everything hygenic. I'll have a look and get back to you.

Manishearth · Apr 19, 2018

I think there you just want to store the argument ident in self.args and use that? You need to carry around the gensymed ident if you need to reuse it.

Manishearth · Apr 19, 2018

Manishearth reviewed Apr 19, 2018

View changes

Manishearth · Apr 19, 2018

src/test/run-pass-fulldeps/deriving-hygiene.rs

+pub const state: u8 = 1;
+pub const cmp: u8 = 1;
+
+#[derive(Ord,Eq,PartialOrd,PartialEq,Debug,Decodable,Encodable,Hash)]


nit: spaces between commas

(also change the commit message to just "test deriving hygiene")

(noted, although I was advised on IRC that I should squash the commits before merging anyway so I've been giving the commit messages a little less care – let me know if that's not the case)

You're free to, but I personally prefer them to stay logically separate if possible, provided they build individually.

So squash the fixups together into logical changes, but you can leave them separate otherwise. Or just squash it all if you want, either way.

Manishearth · Apr 19, 2018

Manishearth approved these changes Apr 19, 2018

View changes

zofrex · Apr 19, 2018

Sorting out the comments is... confusing. The eq example was simple enough but many of the other usage examples it's unclear to me what they should say - given these names are now a parameter, rather than all __arg_1 __arg_2 etc. Could use some opinions there.

I made a first stab at testing this. Do you want separate tests for each trait, or all at once like this?

When it comes to the __args in format, I'm lost. I can't figure out how to get that code emitted in the first place (e.g. not showing up with --pretty=expanded, not conflicting with any consts I declare). I tried:

let s = format!("{}", ());

Manishearth · Apr 19, 2018

All at once is fine.

zofrex · Apr 24, 2018

zofrex reviewed Apr 24, 2018

View changes

zofrex · Apr 24, 2018

src/libsyntax_ext/format.rs

@@ -543,6 +543,10 @@ impl<'a, 'b> Context<'a, 'b> {
        let mut pats = Vec::new();
        let mut heads = Vec::new();

+        let names_pos: Vec<_> = (0..self.args.len()).map(|i| {


This had to go all the way up here because... something to do with E0382. Self gets moved into the expression below with pieces? This can go further down if we have a mutable array in the loop and push names onto it, but I assumed we would prefer to avoid mutability.

zofrex · Apr 24, 2018

zofrex reviewed Apr 24, 2018

View changes

zofrex · Apr 24, 2018

src/libsyntax_ext/format.rs

@@ -543,6 +543,10 @@ impl<'a, 'b> Context<'a, 'b> {
        let mut pats = Vec::new();
        let mut heads = Vec::new();

+        let names_pos: Vec<_> = (0..self.args.len()).map(|i| {
+            self.ecx.ident_of(&format!("arg{}", i)).gensym()


Bikeshed opportunity: arg0 or arg_0?

shrug this is never exposed so it doesn't matter

Manishearth · Apr 24, 2018

Manishearth approved these changes Apr 24, 2018

View changes

Manishearth · Apr 24, 2018

@bors r+

thanks!

bors · Apr 24, 2018

📌 Commit 27b0f1e has been approved by Manishearth

zofrex · Apr 24, 2018

Wait, there's more! (Sorry, was still working on the tests, didn't think this was r+ material yet)

Manishearth · Apr 24, 2018

Oh, right, the format stuff can also break things.

@bors r+

bors · Apr 24, 2018

📌 Commit d6feab6 has been approved by Manishearth

zofrex · Apr 24, 2018

Awesome! It's good to get this in, but just FYI there's still some more to be done - the __self bits turned out to also be tricky to handle, and a bunch of comments in the code are now wrong. I'll try to improve those and file new PRs for them (although I don't mind if someone else does them, of course) – can I bug you for help if I need it? :)

Manishearth · Apr 24, 2018

Yeah, I'd prefer that be done in a followup. Feel free to ask me stuff!

bors · Apr 24, 2018

⌛️ Testing commit d6feab6 with merge 4e4efe0...

bors · Apr 25, 2018

💔 Test failed - status-travis

rust-highfive · Apr 25, 2018

The job dist-x86_64-linux of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:14:54] warning: spurious network error (1 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/cargo-vendor/0.1.4/download`, got 503
[01:14:55] error: failed to get 200 response from `https://crates.io/api/v1/crates/cargo-vendor/0.1.4/download`, got 503
[01:14:55] 
[01:14:55] 
[01:14:55] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "install" "--force" "--debug" "--vers" "0.1.4" "cargo-vendor"
[01:14:55] 
[01:14:55] 
[01:14:55] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --host x86_64-unknown-linux-gnu --target x86_64-unknown-linux-gnu
[01:14:55] Build completed unsuccessfully in 1:10:45

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rust-highfive · Apr 25, 2018

The job dist-x86_64-linux of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:14:54] warning: spurious network error (1 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/cargo-vendor/0.1.4/download`, got 503
[01:14:55] error: failed to get 200 response from `https://crates.io/api/v1/crates/cargo-vendor/0.1.4/download`, got 503
[01:14:55] 
[01:14:55] 
[01:14:55] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "install" "--force" "--debug" "--vers" "0.1.4" "cargo-vendor"
[01:14:55] 
[01:14:55] 
[01:14:55] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --host x86_64-unknown-linux-gnu --target x86_64-unknown-linux-gnu
[01:14:55] Build completed unsuccessfully in 1:10:45

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nagbot-rs · Apr 25, 2018

@bors: retry * crates.io was down

…

On Tue, Apr 24, 2018 at 7:44 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/370824219?utm_source=github_status&utm_medium=notification> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#49986 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AaPN0FSQgZ1g8e8U21ofL5bjXzqvSu-aks5tr8bxgaJpZM4TVn34> . -- You received this message because you are subscribed to the Google Groups "rust-ops" group. To unsubscribe from this group and stop receiving emails from it, send an email to ***@***.*** To post to this group, send email to ***@***.*** To view this discussion on the web visit https://groups.google.com/d/ msgid/rust-ops/rust-lang/rust/pull/49986/c384124495%40github.com <https://groups.google.com/d/msgid/rust-ops/rust-lang/rust/pull/49986/c384124495%40github.com?utm_medium=email&utm_source=footer> . For more options, visit https://groups.google.com/d/optout.

bors · Apr 25, 2018

@nagbot-rs: 🔑 Insufficient privileges: not in try users

Manishearth · Apr 25, 2018

@bors retry

@rust-lang/infra, yer bot be broken

bors · Apr 25, 2018

⌛️ Testing commit d6feab6 with merge 0c5740f...

bors · Apr 25, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Manishearth
Pushing 0c5740f to master...

Provide better names for builtin deriving-generated attributes

Loading status checks…

4ae9488

pietroalbini added the S-waiting-on-review label Apr 16, 2018

rust-highfive assigned petrochenkov Apr 16, 2018

rust-highfive assigned Manishearth and unassigned petrochenkov Apr 16, 2018

Manishearth referenced this pull request Apr 16, 2018
Closed
Don't use underscores in argument names for derive output #49676

pietroalbini added S-waiting-on-author and removed S-waiting-on-review labels Apr 16, 2018

Manishearth and others added some commits Apr 5, 2018

Don't run unused variable pass for stuff generated by #[derive()]

24d410a

Don't prepend deriving-generated attributes with _

Loading status checks…

a3241d1

zofrex added some commits Apr 19, 2018

Gensym remaining identifiers

b9b650c

Fix some of the __names in the comments

1a72d6d

bors added S-waiting-on-bors and removed S-waiting-on-author labels Apr 24, 2018

Test format hygiene

Loading status checks…

d6feab6

bors added S-waiting-on-review and removed S-waiting-on-bors labels Apr 25, 2018

bors added S-waiting-on-bors and removed S-waiting-on-review labels Apr 25, 2018

bors merged commit d6feab6 into rust-lang:master Apr 25, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors referenced this pull request Apr 25, 2018
Merged
Ensure derive(PartialOrd) is no longer accidentally exponential #50011

rust-lang/rust

Join GitHub today

Provide better names for builtin deriving-generated attributes #49986

Conversation

zofrex commented Apr 15, 2018

pietroalbini added the S-waiting-on-review label Apr 16, 2018

This comment has been minimized.

pietroalbini commented Apr 16, 2018

rust-highfive assigned petrochenkov Apr 16, 2018

This comment has been minimized.

zofrex commented Apr 16, 2018

rust-highfive assigned Manishearth and unassigned petrochenkov Apr 16, 2018

Manishearth referenced this pull request Apr 16, 2018

Manishearth approved these changes Apr 16, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

zofrex Apr 16, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Apr 16, 2018

This comment has been minimized.

Manishearth commented Apr 16, 2018

This comment has been minimized.

zofrex commented Apr 16, 2018

pietroalbini added S-waiting-on-author and removed S-waiting-on-review labels Apr 16, 2018

This comment has been minimized.

Manishearth commented Apr 16, 2018 • edited

Manishearth and others added some commits Apr 5, 2018

This comment has been minimized.

zofrex commented Apr 19, 2018

This comment has been minimized.

zofrex commented Apr 19, 2018 • edited

This comment has been minimized.

Manishearth commented Apr 19, 2018

This comment has been minimized.

Manishearth commented Apr 19, 2018

This comment has been minimized.

zofrex commented Apr 19, 2018

This comment has been minimized.

Manishearth commented Apr 19, 2018

This comment has been minimized.

Manishearth commented Apr 19, 2018

zofrex added some commits Apr 19, 2018

Manishearth reviewed Apr 19, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth approved these changes Apr 19, 2018 View changes

This comment has been minimized.

zofrex commented Apr 19, 2018

This comment has been minimized.

Manishearth commented Apr 19, 2018

zofrex reviewed Apr 24, 2018 View changes

This comment has been minimized.

zofrex reviewed Apr 24, 2018 View changes

This comment has been minimized.

This comment has been minimized.

Manishearth approved these changes Apr 24, 2018 View changes

This comment has been minimized.

Manishearth commented Apr 24, 2018

This comment has been minimized.

bors commented Apr 24, 2018

bors added S-waiting-on-bors and removed S-waiting-on-author labels Apr 24, 2018

This comment has been minimized.

zofrex commented Apr 24, 2018

This comment has been minimized.

Manishearth commented Apr 24, 2018

This comment has been minimized.

bors commented Apr 24, 2018

This comment has been minimized.

zofrex commented Apr 24, 2018

This comment has been minimized.

Manishearth commented Apr 24, 2018

This comment has been minimized.

bors commented Apr 24, 2018

Manishearth approved these changes Apr 16, 2018

View changes

zofrex Apr 16, 2018 •

edited

Manishearth commented Apr 16, 2018 •

edited

zofrex commented Apr 19, 2018 •

edited

Manishearth reviewed Apr 19, 2018

View changes

Manishearth approved these changes Apr 19, 2018

View changes

zofrex reviewed Apr 24, 2018

View changes

zofrex reviewed Apr 24, 2018

View changes

Manishearth approved these changes Apr 24, 2018

View changes

bors merged commit `d6feab6` into rust-lang:master Apr 25, 2018
2 checks passed