add trait impls to proc_macro::Ident

[Qelxiros]

ACP: rust-lang/libs-team#573

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[Qelxiros]

Clippy context: An aux crate (proc_macros.rs) was using Ident::to_string before comparing to a string literal, so I updated it to use the PartialEq implementation added in this PR.

[petrochenkov]

I suggest splitting this into one PR with (insta-stable) API changes, and then another PR with implementation optimizations.

Also, PartialEq impl for Ident was previously rejected (see #78634 and its linked issue) for the reasons that are still relevant now.
Comparisons with strings are ok, Hash is probably also ok, but not especially useful without Eq.

[Qelxiros]

To clarify, the clippy change was to fix CI.

Thanks for the context! I'd be happy to remove the PartialEq<Self> impl (and Eq, by extension) or modify it to compare spans as well if you think the rest is mergeable, or leave it as is pending further discussion. I'll also link your comment on the ACP for visibility.

[Qelxiros]

@rustbot ready

[petrochenkov]

and then another PR with implementation optimizations

This was about moving the code around and providing the optimized implementation instead of just doing .to_string().
I don't think it's necessary to add the new APIs.

The Hash impl is still questionable because it's inconsistent with the only PartialEq impl, but I'll leave this for the libs team to decide.
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Qelxiros]

@petrochenkov I'm a little confused. Do you mean that this implementation should be reduced to self.to_string() == other.as_ref()? That seems to miss the point of the ACP, which was to compare Idents with Strings (and friends) without an allocation.

[petrochenkov]

Do you mean that this implementation should be reduced to self.to_string() == other.as_ref()?

Yes.
Allocation or not is an implementation detail, that can be changed at any later point without any process or team decisions.

[petrochenkov]

Did you push the changes?
I still see all the unnecessary code moving and optimizations.

[Qelxiros]

@rustbot ready

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Qelxiros]

@rustbot ready

I think I got rid of the unnecessary code moving, but I'm not sure what you're referring to as optimizations. Let me know what, if anything, I missed.

[petrochenkov]

but I'm not sure what you're referring to as optimizations

Everything outside of library/proc_macro/src/lib.rs.

[petrochenkov]

Anyway, I'm sending this to the libs-api team for approval.

This PR adds two public APIs:

• PartialEq with AsRef<str> for Ident
  • this should be ok from my point of view
• Hash for Ident
  • this is at least suspicious, because this Hash impl is inconsistent with the (nonexistent) PartialEq impl for the same type, not sure what is the library policy regarding such impls

[joshtriplett]

We took a brief look at this in today's @rust-lang/libs-api meeting. We didn't look at the Hash impl, but we looked at the PartialEq impl.

We observed that this is providing impl PartialEq<T> for ..., which is a blanket impl that we think would preclude people implementing PartialEq for their own types (without using negative impls). We don't have any other blanket PartialEq<T> impls in the standard library.

It'd be a bit more work, but please consider implementing PartialEq<SpecificType> for each specific type that currently implements AsRef<str>. (A macro should make that easy.)

[dtolnay]

We observed that this is providing impl PartialEq<T> for ..., which is a blanket impl that we think would preclude people implementing PartialEq for their own types (without using negative impls).

proc_macro2::Ident has this impl and it does not preclude having more PartialEq impls downstream.

https://docs.rs/proc-macro2/1.0.101/proc_macro2/struct.Ident.html#impl-PartialEq%3CT%3E-for-Ident
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=91fd406853eede2540a45ec903f70fc6

Since you mentioned a negative impl, I assume that you expected this would only be allowed in combination with a impl !AsRef<str> for MyIdent {}. That is not the case.

The only thing it precludes is a custom PartialEq with some type that also implements AsRef<str> where you want PartialEq to be inconsistent with the standard library's AsRef-based impl, which seems fine.

[dtolnay]

• impl Hash for Ident — I am not sure that this is useful without impl PartialEq<Self> for Ident. Did we have a use case in mind? I am open to also adding PartialEq<Self> (and proc-macro2 already has it) but it was previously discussed and decided against in Implement PartialEq for Ident, PartialEq for TokenStream #51074.

• impl<T> PartialEq<T> for Ident where T: AsRef<str> + ?Sized — this is a breaking change for any type T that already has both PartialEq<T> for proc_macro::Ident and AsRef<str> for T. This is probably not widespread but I would not be surprised if somebody has done this, and if somebody has done it, there is a fair chance it is in some foundational proc macro helper library so the blast radius of breaking it could be unexpectedly big. A crater run might be called for.

[dtolnay]

We discussed this PR in today's standard library API team meeting. Those present were on board with adding this adjusted group of impls:

• impl PartialEq<_> for Ident for some small set of standard library string types, including at least str and String
• impl PartialEq<Ident> for Ident
• impl Eq for Ident
• impl Hash for Ident

Transitivity

We discussed whether ident–string comparisons violate the requirement that PartialEq impls behave transitively. According to the documentation of PartialEq, by symmetry and transitivity, if:

1. ident1 == string is true and
2. ident2 == string is true and
3. a PartialEq impl exists for string == ident2 and
4. a PartialEq impl exists for ident1 == ident2

then ident1 == ident2 must be true. The impl for 3 (impl PartialEq<Ident> for {string}) is not presently being proposed, so the symmetry and transitivity requirements both end up being void, at least among standard library PartialEq impls. But every new standard library PartialEq impl imposes new expectations on the behavior of downstream PartialEq impls arising from transitivity, and this one is no different.

Hygiene

Ignoring the absence of impl 3, one would reasonably expect that ident1 == string and ident2 == string implies ident1 == ident2. This wouldn't be the case if ident–string comparison disregarded spans (which is the only realistic possibility for this comparison) while ident–ident comparison did not.

While "do these idents resolve to the same thing" is sometimes a reasonable thing to want to know, "do these idents have the same Span" is neither that same question nor basically ever what someone would want to know in a proc macro.

We discussed the scenario of idents that have the same span but end up referring to different things after name resolution, which is counterintuitive to people with an incomplete familiarity with Span. An example is:

let ident = Ident::new("X", Span::call_site()); quote! { { const X: i8 = 1; print($ident); } { const X: i8 = 2; print($ident); } }

Because of this kind of thing, "do these idents have the same Span" is not a workable proxy for "do these idents resolve to the same thing". The latter is not something that would be knowable during macro expansion, and it would not make sense for PartialEq comparison on Ident to be the way that we would try to surface this information.

In contrast, "is this ident some specific keyword/symbol that is meaningful to the macro" is the 100% use case and it makes sense for Ident to provide affordances to make this check readable and easy.

AsRef-based

The impl currently implemented in the PR is impl<T> PartialEq<T> for Ident where T: AsRef<str> + ?Sized, which aligns with the impl provided by syn::Ident since 2016 (0.5.0) and proc_macro2::Ident since 2018 (0.4.2).

A subset of the team was hesitant about this generic impl for proc_macro::Ident because it rules out downstream types that could want to have both an AsRef<str> impl and a PartialEq impl based on Ident comparison, not string comparison. Hypothetically like this:

pub struct MyIdent { string: String, ident: proc_macro::Ident, } impl MyIdent { pub fn new(ident: proc_macro::Ident) -> Self { MyIdent { string: ident.to_string(), ident, } } } impl AsRef<str> for MyIdent { fn as_ref(&self) -> &str { &self.string } } impl PartialEq<MyIdent> for proc_macro::Ident { fn eq(&self, other: &MyIdent) -> bool { // more efficient than `*self == other.as_ref()` *self == other.ident } }

While it is difficult to imagine any procedural macro that would be bottlenecked on string comparison performance vs the faster interned identifier integer comparison, avoiding the AsRef-based generic PartialEq impl seemed like the safer route that can achieve consensus more easily. The number of concrete types that people will want to compare against in real-world code is expected to be small.

Hash

The proposed Hash impl is nearly useless without also adding an Eq and PartialEq<Ident>. We are on board with adding this entire group of impls.

Together these make Ident usable in HashSet<Ident> and HashMap<Ident, ...>. A quirk of these collections will be that inserting the same identifier multiple times with different spans will end up being sensitive to insertion order for which span ends up being the one observable in the collection upon iteration.

Ord

Possible future extension: now that PartialEq<Ident> will exist, the impls impl PartialOrd<Ident> for Ident and impl Ord for Ident are unblocked. In proc macros it commonly makes more sense to use Ord-based collections (BTreeSet, BTreeMap) instead of Hash-based (HashSet, HashMap) in order to avoid accidentally generating nondeterministic macro output. Nondeterminism is bad for cache reuse (for build systems that support such a thing) and for debuggability of macro-generated code using cargo expand/diff. But let us defer PartialOrd and Ord to a separate PR.