add trait impls to proc_macro::Ident by Qelxiros · Pull Request #146553 · rust-lang/rust (original) (raw)

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

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 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 for MyIdent { fn as_ref(&self) -> &str { &self.string } }

impl PartialEq 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.