Introduce ModernIdent type to unify macro 2.0 hygiene handling #144439

xizheyin · 2025-07-25T09:15:03Z

This pr introduce ModernIdent type to unify macro 2.0 hygiene handling

Added ModernIdent type. Wraps Ident and automatically calls normalize_to_macros_2_0()
Unified identifier normalization. Replaced scattered ident.normalize_to_macros_2_0() calls with ModernIdent::new(ident)

r? @petrochenkov

rustbot · 2025-07-25T09:15:09Z

HIR ty lowering was modified

cc @fmease

fee1-dead · 2025-07-25T16:29:23Z

What problem is this intended to solve? ModernIdent seems like a quite confusing name.

petrochenkov · 2025-07-25T16:11:32Z

compiler/rustc_span/src/symbol.rs

+/// different macro 2.0 macros.
+// FIXME: Use more often
+#[derive(Copy, Clone, Eq, PartialEq, Hash)]
+pub struct ModernIdent(pub Ident);


Suggested change

pub struct ModernIdent(pub Ident);

pub struct Macros20NormalizedIdent(pub Ident);

The modern / legacy terminology for macros 2.0 / macro_rules is not used anymore.

petrochenkov · 2025-07-25T16:12:52Z

compiler/rustc_span/src/symbol.rs

+    }
+}
+
+impl std::borrow::Borrow<Ident> for ModernIdent {


Is there any specific reason for this impl?

We can convert it into Ident by passing a reference, and some parts of the code don't have to be changed to ident.0

Do I need to delete this?

The by-ref coercions are enabled by Deref, if I understand the case correctly.
Also, Idents should usually be passed by value, unless some generic trait requires a reference.

agree with petrochenkov here. also I believe borrow generally should not be implemented for two types where one has more invariants than the other.

petrochenkov · 2025-07-25T16:15:33Z

compiler/rustc_hir_analysis/src/coherence/inherent_impls_overlap.rs

@@ -65,8 +65,7 @@ impl<'tcx> InherentOverlapChecker<'tcx> {
    fn compare_hygienically(&self, item1: ty::AssocItem, item2: ty::AssocItem) -> bool {
        // Symbols and namespace match, compare hygienically.
        item1.namespace() == item2.namespace()
-            && item1.ident(self.tcx).normalize_to_macros_2_0()
-                == item2.ident(self.tcx).normalize_to_macros_2_0()
+            && ModernIdent::new(item1.ident(self.tcx)) == ModernIdent::new(item2.ident(self.tcx))


I don't think replacing the normalize_to_macros_2_0 calls in general is an improvement.
The new ident type makes sense only if it's stored in something or passed to something having an invariant.

That makes sense, so I need to delete some unnecessary substitutions.

petrochenkov · 2025-07-25T16:19:14Z

compiler/rustc_span/src/symbol.rs

+impl ModernIdent {
+    #[inline]
+    pub fn new(ident: Ident) -> Self {
+        Self(ident.normalize_to_macros_2_0())


Suggested change

Self(ident.normalize_to_macros_2_0())

ModernIdent(ident.normalize_to_macros_2_0())

Could you avoid using Self outside of generic code?

petrochenkov · 2025-07-25T16:20:30Z

compiler/rustc_span/src/symbol.rs

+    }
+
+    #[inline]
+    pub fn with_dummy_span(name: Symbol) -> Self {


This method isn't used anywhere.

petrochenkov · 2025-07-25T16:25:58Z

compiler/rustc_resolve/src/macros.rs

@@ -1139,14 +1139,16 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
        }
    }

-    pub(crate) fn check_reserved_macro_name(&self, ident: Ident, res: Res) {
+    pub(crate) fn check_reserved_macro_name(&self, ident: ModernIdent, res: Res) {


Suggested change

pub(crate) fn check_reserved_macro_name(&self, ident: ModernIdent, res: Res) {

pub(crate) fn check_reserved_macro_name(&self, ident: Ident, res: Res) {

This function would take a Symbol if it not needed the span for the error.
In general, for displaying spans non-normalized idents are better.

compiler/rustc_resolve/src/lib.rs

compiler/rustc_resolve/src/build_reduced_graph.rs

compiler/rustc_resolve/src/late.rs

compiler/rustc_span/src/symbol.rs

petrochenkov · 2025-07-25T17:01:09Z

In general, I have mixed feelings about both this and MacroRulesNormalizedIdent.
It clearly can potentially catch some mistakes or inefficiencies (like it did in fn define_macro, I think), on the other hand it can only be used opportunistically because most places either do not care about the normalization, or care but the normalization should be different in different cases (like in rib.bindings, I guess).

xizheyin

In this version, I removed some unnecessary uses.
The modifications are mainly centered around two fields.

BindingKey::ident
Resolver::extern_prelude

xizheyin · 2025-07-27T08:39:38Z

compiler/rustc_span/src/symbol.rs

+    // For search in index map for `Ident` as a index key
+    pub unsafe fn new_without_normalize(ident: Ident) -> Self {
+        Macros20NormalizedIdent(ident)
+    }


In the last commit, I implemented Borrow to look up in IndexMap<Macro20NormalizedIdent> with Ident as key. But in the new version, I implemented this function to temporarily convert the Ident to Macro20NormalizedIdent without normalizing it to match the type. And I also apply this function to some Idents that were not normalized using normalize_to_macros_2_0() in the original code.

I'll mark three places where I use this function and I'm not sure if I should use new or it.

xizheyin · 2025-07-27T08:40:05Z

compiler/rustc_resolve/src/check_unused.rs

@@ -203,7 +203,7 @@ impl<'a, 'ra, 'tcx> UnusedImportCheckVisitor<'a, 'ra, 'tcx> {
            if self
                .r
                .extern_prelude
-                .get(&extern_crate.ident)
+                .get(&unsafe { Macros20NormalizedIdent::new_without_normalize(extern_crate.ident) })


Here use new_without_normalize for indexing

xizheyin · 2025-07-27T08:40:27Z

compiler/rustc_resolve/src/diagnostics.rs

+        let from_item = self
+            .extern_prelude
+            .get(&unsafe { Macros20NormalizedIdent::new_without_normalize(ident) })
+            .is_none_or(|entry| entry.introduced_by_item);


Here also for Indexing

xizheyin · 2025-07-27T08:42:24Z

compiler/rustc_resolve/src/lib.rs

-            .map(|(name, _)| (Ident::from_str(name), Default::default()))
+            .map(|(name, _)| {
+                (
+                    unsafe {
+                        Macros20NormalizedIdent::new_without_normalize(Ident::from_str(name))
+                    },
+                    Default::default(),


Here will return extern_prelude which petrochenkov suggest to convert to Macro20NormalizedIdent.

xizheyin · 2025-07-27T08:46:51Z

compiler/rustc_resolve/src/lib.rs

 struct BindingKey {
    /// The identifier for the binding, always the `normalize_to_macros_2_0` version of the
    /// identifier.
-    ident: Ident,
+    ident: Macros20NormalizedIdent,


Main Change(1/2)

xizheyin · 2025-07-27T08:46:57Z

compiler/rustc_resolve/src/lib.rs

-    extern_prelude: FxIndexMap<Ident, ExternPreludeEntry<'ra>>,
+    extern_prelude: FxIndexMap<Macros20NormalizedIdent, ExternPreludeEntry<'ra>>,


Main change (2/2)

bors · 2025-07-27T16:51:48Z

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

Signed-off-by: xizheyin <[email protected]>

bors · 2025-07-30T04:04:23Z

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

rustbot assigned petrochenkov Jul 25, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 25, 2025

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 25, 2025

petrochenkov reviewed Jul 25, 2025

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2025

xizheyin commented Jul 25, 2025

View reviewed changes

compiler/rustc_span/src/symbol.rs Outdated Show resolved Hide resolved

xizheyin force-pushed the symbol-rs branch from bc34960 to 868ecfb Compare July 27, 2025 08:32

xizheyin commented Jul 27, 2025

View reviewed changes

Introduce ModernIdent type to unify macro 2.0 hygiene handling

d2ad9c8

Signed-off-by: xizheyin <[email protected]>

xizheyin force-pushed the symbol-rs branch from 868ecfb to d2ad9c8 Compare July 28, 2025 02:41

	pub struct ModernIdent(pub Ident);
	pub struct Macros20NormalizedIdent(pub Ident);

	Self(ident.normalize_to_macros_2_0())
	ModernIdent(ident.normalize_to_macros_2_0())

	pub(crate) fn check_reserved_macro_name(&self, ident: ModernIdent, res: Res) {
	pub(crate) fn check_reserved_macro_name(&self, ident: Ident, res: Res) {

		extern_prelude: FxIndexMap<Ident, ExternPreludeEntry<'ra>>,
		extern_prelude: FxIndexMap<Macros20NormalizedIdent, ExternPreludeEntry<'ra>>,

Introduce ModernIdent type to unify macro 2.0 hygiene handling #144439

Are you sure you want to change the base?

Introduce ModernIdent type to unify macro 2.0 hygiene handling #144439

Conversation

xizheyin commented Jul 25, 2025

Uh oh!

rustbot commented Jul 25, 2025

Uh oh!

fee1-dead commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Jul 25, 2025

Uh oh!

xizheyin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jul 27, 2025

Uh oh!

bors commented Jul 30, 2025

Uh oh!

Uh oh!