Implement PartialEq for proc_macro::Ident == strings

[dtolnay]

impl PartialEq<str> for Ident

impl PartialEq<String> for Ident

impl<'a, T> PartialEq<&'a T> for Ident
where
    T: ?Sized,
    Ident: PartialEq<T>

Closes #51074.

We avoid implementing PartialEq<Ident> because "comparison in vacuum doesn't generally make sense in presence of hygiene and it became a source of bugs" --- see #51074 (comment). However, comparing an Ident to a string is "probably explicit enough" --- see #51074 (comment).

The use case is to support attribute parsing, among other things. For example parsing #[serde(rename = "...")] might involve if ident == "serde" and if ident == "rename".

[rust-highfive]

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

[est31]

👍 Recently I've looked for an allocation free method for comparing idents with str's and couldn't find one. Had to use to_string which allocates.

[petrochenkov]

I've nominated both this PR and the char PR for the libs team since they are instant stabilizations.
Both look reasonable to me.

[dtolnay]

@rfcbot fcp merge

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @KodrAus
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

[c410-f3r]

cc #59070

[est31]

I thought about this a little. I wonder about raw vs non-raw idents and whether there should be something to accommodate for it. As in, should an Ident r#hello compare equal with "hello" or not.

If you are checking for keywords, then you don't want them to compare equal to each other. If you are checking for variable/function names etc, then usually you want r#hello be equivalent to "hello". So the problem is not solved by changing defaults (making r#hello compare equal with "hello").

In my puny little proc macro that I'm writing, I'm mainly comparing with keywords, so having them be not equal is good for me, but maybe some folks want to compare to function/variable names and they'll have to do if id == "hello" || id == "r#hello".

Maybe there could be a function like fn ignore_rawness(&self) -> IdentRawnessIgnored<'a> which impl's eq with strings as well, but ignores the rawness? There might also be "raw keywords" in the future which always eval to keywords, making it easier to use new keywords. Then maybe a different name would be nicer. I think both can be added backwards compatibly, and I think id == "hello" is a good shorthand for id.to_string() == "hello", so this PR is a step into the right direction. But important to keep this in mind.

[dtolnay]

We do pretty much that in Syn. There is a function unraw (https://docs.rs/syn/1.0.48/syn/ext/trait.IdentExt.html#tymethod.unraw) with signature &Ident -> Ident, so e.g. you'd write ident.unraw() == "value".

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[petrochenkov]

I've just checked and looks like the standard library doesn't currently any have any PartialEq impls recursively unwrapping references like #78634 (comment) does.

[dtolnay]

I am not dead set on that exact impl signature, but I have found some level of reference unwrapping quite key in practice specifically for Ident. proc_macro2 accomplishes the reference unwrapping by having:

impl<T> PartialEq<T> for Ident
where
    T: ?Sized + AsRef<str>;

where String and str obviously implement AsRef<str>, and then &U implements AsRef<str> whenever U does, unwrapping any number of layers of references via this impl:

impl<U, V> AsRef<V> for &'_ U
where
    U: ?Sized + AsRef<V>,
    V: ?Sized;

However I felt AsRef<str> was maybe too vague of a bound to use for proc_macro::Ident and preferred requiring the rhs to provide a PartialEq explicitly instead in order to compare with Ident.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[petrochenkov]

Could you add some tests for raw identifiers?

[bors]

☔ The latest upstream changes (presumably #78636) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[m-ou-se]

I've just checked and looks like the standard library doesn't currently any have any PartialEq impls recursively unwrapping references like #78634 (comment) does.

This concern was discussed in the libs team meeting of 2020-11-18. Although this hasn't been done before in std, the consensus was that this is fine in this proc_macro addition.

(Thanks for pointing it out!)

[petrochenkov]

Closing due to inactivity.