search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
94.81k stars 12.22k forks source link

Tracking issue for `concat_idents` #29599

Open aturon opened 8 years ago

aturon commented 8 years ago

Tracks stabilization for the concat_idents macro.

Update(fmease): Please see https://github.com/rust-lang/rust/issues/124225#issue-2255069330 (an alternative to this feature).

retep998 commented 8 years ago

concat_idents is so fundamentally useless at the moment because macros cannot be used in ident positions.

ghost commented 8 years ago

Yeah it is pretty much useless right now (see #12249 #13294).

nrc commented 8 years ago

Relevant blog for future plans: http://www.ncameron.org/blog/untitledconcat_idents-and-macros-in-ident-position/

eddyb commented 8 years ago

A solution that has been thrown around on IRC (but never ended up anywhere else AFAIK): Have a macro invocation form for "early expansion", i.e. the following would work without macros in ident position:

macro_rules! wrap {
    ($name:ident) => { struct concat_idents!!(Wrapped, $name)($name); }
}

The "early expansion" macro_name!!(args) syntax was originally $*macro_name!(args) and is a subject of bikeshed.

If we move towards having all macros produce token streams that are parsed on-demand, such "early expansion" could be used in many more locations without adding macro support for each one. Generic parameters, ADT fields, function signatures and match arms come to mind - there are so many recursive push-down hacks macros can end up with, just to construct lists of things and whatnot, that would simply not be necessary with "early expansion".

The only disadvantage is that concat_idents!! and friends would only work inside the RHS of macro_rules!, but I don't really see why you would need to use such a capability outside of a macro.

skade commented 8 years ago

https://github.com/mikkyang/rust-blas uses it for quick notation of FFI functions that follow a certain scheme: https://github.com/mikkyang/rust-blas/blob/master/src/prefix.rs https://github.com/mikkyang/rust-blas/blob/master/src/matrix/ll.rs

skade commented 8 years ago

Well, given that https://github.com/mikkyang/rust-blas/issues/12 is a nicer solution within the current language, I'd also like to raise my hand for "useless".

nrc commented 8 years ago

I think we should never stabilise the current version - as others have noted it is useless.

aidanhs commented 8 years ago

Since this is the tracking issue, other who stumble across it might be interested in https://github.com/SkylerLipthay/interpolate_idents which works on nightly rust.

retep998 commented 8 years ago

Unfortunately that solution will always depend on nightly Rust (until the day that syntax extensions become stable and pigs fly).

skade commented 8 years ago

Given that the consensus seems to be "useless" and theres nicer solutions (e.g. [1], maybe we can remove this feature?

[1] https://github.com/mikkyang/rust-blas/pull/12/files

eddyb commented 8 years ago

See rust-lang/rfcs#1628.

aidanhs commented 8 years ago

@retep998 yes, just noting it as a stopgap.

@skade it's the implementation rather than the idea that's useless. If concat_idents is removed rather than fixed, something else needs to fill its place (your solution doesn't work for all use-cases). I like the look of the rfc @eddyb linked, will follow that.

skade commented 8 years ago

@aidanhs that still means that we should drop the feature to make sure no one uses it.

I agree that my solution doesn't cover all edge-cases, but all uses that I've currently seen in the wild. Also, but this is outside of my competence to decide, I think this is a perfect case for the use of a code generator.

aidanhs commented 8 years ago

@skade afaict nobody can use it, so that's not a big concern...but equally there's no reason to keep it around. My main motivation for leaving it was as a reminder that people do want the functionality, but I'm content to follow the RFC and have changed my position to "I don't mind either way".

Regarding use cases, here's the motivating example that brought me to this issue in the first place. Codegen's awesome for things like phf, but it feels a bit like overkill when I just want to generate some repetitive methods to extract tuples from enum variants. I guess it's down to personal preference.

ExpHP commented 6 years ago

An amusing fact about the current implementation of concat_idents! is that it will accept an empty argument list, making it possible to construct the empty string as an identifier:

error[E0425]: cannot find value `` in this scope
 --> src/main.rs:5:5
  |
5 |     concat_idents!();
  |     ^^^^^^^^^^^^^^^^^ not found in this scope

(good luck actually doing anything with it)

petrochenkov commented 6 years ago

construct the empty string as an identifier

Empty string is used as a reserved identifier with special meaning in several places in the compiler, so this can potentially cause issues.

jonhoo commented 6 years ago

The linked RFC has been closed as postponed. Should concat_idents! now be removed? Is there any chance of seeing something in its place that supports generating identifiers?

durka commented 6 years ago

The RFC should be reopened. It never should have been closed, as no better solution was proposed.

skade commented 6 years ago

@durka I don't agree. There's ample reason to expect a new one when moving towards macros 2.0. RFCs are not necessarily closed to be replaced immediately, they are closed to stop following that line of discussion. Future RFCs might refer to it.

alexreg commented 6 years ago

I'm with @durka on this.

eddyb commented 6 years ago

@skade I don't think much has happened regarding macros 2.0 design except for maybe some implementation experience - e.g. @jseyfried might be aware of potential issues with early expansion.

durka commented 6 years ago

@skade, we are moving towards macros 2.0 as far as I can tell. rust-lang/rfcs#1584 was merged (though the promised follow-ups didn't seem to come) and #![feature(decl_macro)] exists on nightly. When is the appropriate time to reopen all the concerns that were tabled as "wait for macros 2.0"?

skade commented 6 years ago

@eddyb @durka that is both true, my point is that this probably merits a new RFC, based on this one, except of just reopening.

dtolnay commented 6 years ago

I implemented a stable concat_idents approach that works with any compiler >=1.15.0. https://github.com/dtolnay/mashup

alexreg commented 6 years ago

@dtolnay Black magic! Good job though. :-) Does Macros 1.2 support this yet? CC @jseyfried

ExpHP commented 6 years ago

Does Macros 1.2 support this yet?

If Macros 1.2 added support for any form of early expansion, everybody would know about it. I mean, it'd make international news.

alexreg commented 6 years ago

Hah. I know it was being discussed properly at one point... guess it never got too far!

dtolnay commented 6 years ago

I would like to propose stabilizing concat_idents! as it currently exists. As discussed at length in this thread, concat_idents does not solve all use cases for concatenated identifiers especially as a consequence of no macro expansion in identifier position. Nevertheless, this macro does a useful thing and has its uses for putting together certain types of convenient APIs.

Basically I don't feel the need to push for macro expansion in more places before stabilizing use of concat_idents in expression position.

For more elaborate use cases of concatenated identifiers I believe directing people to use mashup is acceptable.

@rfcbot fcp merge

rfcbot commented 6 years ago

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

joshtriplett commented 6 years ago

Without necessarily objecting to this: how difficult would it be to modify the existing concat_idents! in rust to have the functionality that people desire? I realize that doing so does require unstable compiler features to implement, but so do many other stable interfaces in the Rust standard library.

ExpHP commented 6 years ago

Stabilization? This? Really?

Uhh, in that case, I guess I'll throw up a PR to fix the empty identifier bug (since I basically found it by looking at the source code).

alexreg commented 6 years ago

@dtolnay Sounds reasonable. Using it in identifier places can come when my PR for hygiene escaping lands. :-)

durka commented 6 years ago

Maybe with a big banner in the docs saying "this probably isn't what you want!!!" and pointing to mashup.

On Wed, May 2, 2018 at 8:30 PM, Michael Lamparski notifications@github.com wrote:

Stabilization? This? Really?

Uhh, in that case, I guess I'll throw up a PR to fix the empty identifier bug (since I basically found it by looking at the source code).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/29599#issuecomment-386161726, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC3n7oAQwOdgziD9t24XKIwm4chVRPlks5tuk-7gaJpZM4GcPel .

eddyb commented 6 years ago

@joshtriplett What do you mean? This macro can't be implemented in a library, it's already built-in. However, macro invocations are not allowed in identifier positions which simplifies many things (how do you parse a macro invocation when its name could be produced by another macro?), and this is where "early expansion" comes in, because then you're "just" expanding arbitrary tokens, not ASTs.

durka commented 6 years ago

@joshtriplett I don't think concat_idents is at fault, really. The issue is that macros can't be expanded in certain positions. Something would have to change around that, or around eager expansion, to be able to use concat_idents for something like constructing the names of getter/setter functions. The other issue is hygiene, but that is going to be fixed with proc macros in another way (punted for now).

alexreg commented 6 years ago

@eddyb Ah, I thought it was just a hygiene problem... Looks like it's a hygiene and early-expansion problem.

@durka Yep, I'm working on the hygiene part. We've got what seems like a pretty good approach now, but we're actually going to implement two and feature-gate them, then experiment with them.

mark-i-m commented 6 years ago

Perhaps change the name to reflect that it probably isn't what you want?

On Wed, May 2, 2018, 8:08 PM Alexander Regueiro notifications@github.com wrote:

@eddyb https://github.com/eddyb Ah, I thought it was just a hygiene problem... Looks like it's a hygiene and early-expansion problem.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/29599#issuecomment-386166687, or mute the thread https://github.com/notifications/unsubscribe-auth/AIazwBfVVTrITMp1JxyJRqj7XjOVYs3Cks5tulidgaJpZM4GcPel .

ExpHP commented 6 years ago

Is there a reason why it can't be used in patterns?

error: non-pattern macro in pattern position: concat_idents
 --> src/main.rs:3:9
  |
3 |     let concat_idents!(fn) = 4;
  |         ^^^^^^^^^^^^^
ExpHP commented 6 years ago

Oh and FYI you can construct keywords as identifiers, too =P

Edit: I'm dumb, that's probably intentional for the sake of raw identifiers

cramertj commented 6 years ago

@rfcbot concern naming

Practically every use I've had for concat_idents has required being able to use macros in identifier position, so if I discovered a stable macro called concat_idents I'd certainly expect this to work. Can we come up with a better name for the macro that is suggestive of its limited usability, while also leaving the door open to a macro that "does what you want" such as mashup being called concat_idents!?

sfackler commented 6 years ago

The macro does what it says on the tin - it concatenates idents. The changes required to make it work in positions it doesn't yet aren't in the macro, they're in the rest of the compiler.

alexreg commented 6 years ago

@cramertj mashup is a big hack, right now, as I see. Anyway, it's a 3rd-party crate, and I don't see why we should be concerned about official Rust identifiers clashing with 3rd-party ones. Once eager-expansion and hygiene land, it will do what we all want.

ExpHP commented 6 years ago

@sfackler if that was directed to me, pattern macros already exist.

If not, uh... carry on!

durka commented 6 years ago

I'm not confident eager expansion is coming, and not sure what "hygiene landing" means.

On Wed, May 2, 2018 at 10:45 PM, Alexander Regueiro < notifications@github.com> wrote:

@cramertj https://github.com/cramertj mashup is a big hack, right now, as I see. Anyway, it's a 3rd-party crate, and I don't see why we should be concerned about official Rust identifiers clashing with 3rd-party ones. Once eager-expansion and hygiene land, it will do what we all want.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/29599#issuecomment-386178433, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC3n0wf8OBy8BAikrBCnRaLx2l2iqUZks5tum9IgaJpZM4GcPel .

durka commented 6 years ago

Are there actual uses that can't be replaced by mashup? Mashup is a hack for now, but it should be fine once function-like proc macros land.

Unscientifically, from the first page of the search results @dtolnay linked, 7 repos don't actually use the macro (presumably they added the feature and then discovered it was useless).

Maybe concat_idents is tantalizing but useless and should be deprecated.

On Wed, May 2, 2018 at 10:46 PM, Michael Lamparski <notifications@github.com

wrote:

@sfackler https://github.com/sfackler if that was directed to me, pattern macros already exist.

If not, uh... carry on!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/29599#issuecomment-386178524, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC3n01QOGA8R2UaJrcUTzzWB5qmAJUpks5tum9vgaJpZM4GcPel .

retep998 commented 6 years ago

I'd love to see someone do a demonstration where they adjust winapi's macros to use mashup and eliminate the redundant inputs I have to specify due to the lack of a useful concat_idents.

joshtriplett commented 6 years ago

@eddyb I can understand not being able to produce a macro invocation with a macro, for instance. But why does that prevent, for instance, generating the name of a function or variable?

dtolnay commented 6 years ago

Sorry about the search results @durka, obviously :broken_heart: GitHub search. I clicked through the first 8 pages and found the following diverse and wonderful use cases. These crates are using concat_idents because it does exactly what is needed for their situation. I am happy to dig through all 27 pages if this does not change your mind about the macro being "tantalizing but useless and should be deprecated."

I believe concat_idents! is a valuable building block for macros, much like stringify!, despite not solving everybody's use cases all the time.

dtolnay commented 6 years ago

Through page 13:

dtolnay commented 6 years ago