Provide better names for builtin deriving-generated attributes #49986

zofrex · 2018-04-15T20:02:39Z

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 · 2018-04-16T09:49:21Z

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 · 2018-04-16T13:59:21Z

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 · 2018-04-16T20:35:52Z

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 · 2018-04-16T20:36:50Z

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 · 2018-04-16T20:37:06Z

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 · 2018-04-16T20:37:46Z

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

Manishearth · 2018-04-16T20:41:05Z

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 · 2018-04-16T21:35:05Z

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

Manishearth · 2018-04-16T22:04:26Z

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

zofrex · 2018-04-19T14:28:44Z

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 · 2018-04-19T17:38:39Z

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 · 2018-04-19T17:45:27Z

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 · 2018-04-19T17:46:27Z

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

zofrex · 2018-04-19T17:49:11Z

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 · 2018-04-19T17:50:38Z

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

Manishearth · 2018-04-19T17:52:38Z

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 · 2018-04-19T20:15:54Z

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.

zofrex · 2018-04-19T20:20:33Z

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 · 2018-04-19T20:22:15Z

All at once is fine.

zofrex · 2018-04-24T20:34:53Z

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 · 2018-04-24T20:35:05Z

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 · 2018-04-24T21:10:19Z

@bors r+

thanks!

bors · 2018-04-24T21:10:20Z

📌 Commit 27b0f1e has been approved by Manishearth

zofrex · 2018-04-24T21:15:17Z

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

Manishearth · 2018-04-24T21:16:33Z

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

@bors r+

bors · 2018-04-24T21:16:34Z

📌 Commit d6feab6 has been approved by Manishearth

zofrex · 2018-04-24T21:32:04Z

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 · 2018-04-24T21:40:13Z

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

bors · 2018-04-24T23:26:17Z

⌛ Testing commit d6feab6 with merge 4e4efe01cc8c74e20fd15662efdca48b3a50d42c...

bors · 2018-04-25T00:44:13Z

💔 Test failed - status-travis

rust-highfive · 2018-04-25T00:44:14Z

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 · 2018-04-25T00:44:22Z

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 · 2018-04-25T01:49:07Z

@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 · 2018-04-25T01:49:08Z

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

Manishearth · 2018-04-25T01:50:53Z

@bors retry

@rust-lang/infra, yer bot be broken

bors · 2018-04-25T01:51:04Z

⌛ Testing commit d6feab6 with merge 0c5740f...

…earth Provide better names for builtin deriving-generated attributes 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`?)

bors · 2018-04-25T04:19:03Z

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

Provide better names for builtin deriving-generated attributes

4ae9488

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 16, 2018

rust-highfive assigned petrochenkov Apr 16, 2018

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

Manishearth mentioned this pull request Apr 16, 2018

Don't use underscores in argument names for derive output #49676

Closed

Manishearth approved these changes Apr 16, 2018

View reviewed changes

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2018

Manishearth and others added 2 commits April 17, 2018 10:39

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

24d410a

Don't prepend deriving-generated attributes with _

a3241d1

zofrex added 2 commits April 19, 2018 20:06

Gensym remaining identifiers

b9b650c

Fix some of the __names in the comments

1a72d6d

Manishearth reviewed Apr 19, 2018

View reviewed changes

Manishearth approved these changes Apr 19, 2018

View reviewed changes

zofrex commented Apr 24, 2018

View reviewed changes

Manishearth approved these changes Apr 24, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2018

Test format hygiene

d6feab6

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 25, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2018

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

bors mentioned this pull request Apr 25, 2018

Ensure derive(PartialOrd) is no longer accidentally exponential #50011

Merged

Provide better names for builtin deriving-generated attributes #49986

Provide better names for builtin deriving-generated attributes #49986

Conversation

zofrex commented Apr 15, 2018

pietroalbini commented Apr 16, 2018

zofrex commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zofrex Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Apr 16, 2018

Manishearth commented Apr 16, 2018

zofrex commented Apr 16, 2018

Manishearth commented Apr 16, 2018 • edited Loading

zofrex commented Apr 19, 2018

zofrex commented Apr 19, 2018 • edited Loading

Manishearth commented Apr 19, 2018

Manishearth commented Apr 19, 2018

zofrex commented Apr 19, 2018

Manishearth commented Apr 19, 2018

Manishearth commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zofrex commented Apr 19, 2018

Manishearth commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Apr 24, 2018

bors commented Apr 24, 2018

zofrex commented Apr 24, 2018

Manishearth commented Apr 24, 2018

bors commented Apr 24, 2018

zofrex commented Apr 24, 2018

Manishearth commented Apr 24, 2018

bors commented Apr 24, 2018

bors commented Apr 25, 2018

rust-highfive commented Apr 25, 2018

rust-highfive commented Apr 25, 2018

nagbot-rs commented Apr 25, 2018 via email

bors commented Apr 25, 2018

Manishearth commented Apr 25, 2018

bors commented Apr 25, 2018

bors commented Apr 25, 2018

zofrex Apr 16, 2018 •

edited

Loading

Manishearth commented Apr 16, 2018 •

edited

Loading

zofrex commented Apr 19, 2018 •

edited

Loading