Skip to content

Builtin deriving-generated attributes should be named better #49967

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.

Sign up for GitHub

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

Jump to bottom
Closed
Manishearth opened this issue Apr 14, 2018 · 1 comment
Closed

Builtin deriving-generated attributes should be named better #49967

Manishearth opened this issue Apr 14, 2018 · 1 comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@Manishearth
Copy link
Member

Currently, these show up as __arg_0, etc in docs:

image

We should provide the ability for builtin derives to specify a string for each argument name, e.g. the args vector here (usage) would be a vector of tuples.

To fix this we also need to generate identifiers hygenically. Currently we generate __arg_0 so that it does not clash with other constants, but instead we should be doing something like ident_of("argumentname").gensym() which we then pass around.

(Ideally, we'd do the same for the other double underscored idents in the deriving code)

@zofrex is working on this
@Manishearth Manishearth added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Apr 14, 2018
@zofrex zofrex mentioned this issue Apr 15, 2018
Merged
bors added a commit that referenced this issue Apr 25, 2018
@bors
Auto merge of #49986 - zofrex:better-derived-argument-names, r=Manish…
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`?)
@nsmaciej
Copy link

nsmaciej commented May 27, 2018 •
edited
Loading

This looks like it can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Manishearth @nsmaciej @ishitatsuyuki