fix(codegen): only add mapping names for PrivateIdentifiers which have changed (#25958)

A `PrivateIdentifier`'s span includes the leading `#`, but the `name` the AST holds does not.

The source map name was read from the whole span and then compared to `name` from AST, so the check which omits the mapping's `name` when it wasn't renamed compared `#foo` against `foo`. It never matched, so a named mapping was created for _every_ private identifier, renamed or not.

This PR fixes that in both Rust `oxc_codegen` and JS `oxc-codegen`, by comparing correctly including the `#`.

This also makes mapping names correct when a normal `Identifier` has been replaced with a `PrivateIdentifier` (`this.foo` -> `this.#foo)`, or vice-versa (`this.#foo` -> `this.foo`). New tests cover both directions.

crates/oxc_codegen/src/gen.rs

@@ -3066,18 +3066,18 @@ impl Gen for AccessorProperty<'_> {
 
 impl Gen for PrivateIdentifier<'_> {
     fn r#gen(&self, p: &mut Codegen, _ctx: Context) {
-        p.add_source_mapping_for_name(self.span, &self.name);
-        p.print_ascii_byte(b'#');
-
-        if let Some(mangled) = p.private_member_mappings.as_ref().and_then(|mappings| {
+        let mangled = p.private_member_mappings.as_ref().and_then(|mappings| {
             p.current_class_ids()
                 .find_map(|class_id| mappings.get(class_id).and_then(|m| m.get(self.name.as_str())))
                 .cloned()
-        }) {
-            p.print_str(mangled.as_str());
-        } else {
-            p.print_str(self.name.as_str());
-        }
+        });
+        // Name the mapping after what is printed, not after what the AST holds, so a mangled name
+        // is recorded as a rename and an unmangled one as no rename at all
+        let name = mangled.as_ref().map_or(self.name.as_str(), |mangled| mangled.as_str());
+
+        p.add_source_mapping_for_private_name(self.span, name);
+        p.print_ascii_byte(b'#');
+        p.print_str(name);
     }
 }
 

crates/oxc_codegen/src/lib.rs

@@ -980,4 +980,19 @@ impl<'a> Codegen<'a> {
     #[inline]
     #[expect(clippy::needless_pass_by_ref_mut, clippy::unused_self)]
     fn add_source_mapping_for_name(&mut self, _span: Span, _name: &str) {}
+
+    /// Add a mapping for a private identifier, printed as `#` followed by `name`.
+    #[cfg(feature = "sourcemap")]
+    fn add_source_mapping_for_private_name(&mut self, span: Span, name: &str) {
+        if let Some(sourcemap_builder) = self.sourcemap_builder.as_mut()
+            && !span.is_empty()
+        {
+            sourcemap_builder.add_source_mapping_for_private_name(self.code.as_bytes(), span, name);
+        }
+    }
+
+    #[cfg(not(feature = "sourcemap"))]
+    #[inline]
+    #[expect(clippy::needless_pass_by_ref_mut, clippy::unused_self)]
+    fn add_source_mapping_for_private_name(&mut self, _span: Span, _name: &str) {}
 }

crates/oxc_codegen/src/sourcemap_builder.rs

@@ -99,6 +99,30 @@ impl<'a> SourcemapBuilder<'a> {
     }
 
     pub fn add_source_mapping_for_name(&mut self, output: &[u8], span: Span, name: &str) {
+        let original_name = self.original_name(span, name);
+        // The token name should be original name.
+        // If it hasn't changed, name should be `None` to reduce `SourceMap` size.
+        let token_name = if original_name == Some(name) { None } else { original_name };
+        self.add_source_mapping(output, span.start, token_name);
+    }
+
+    /// Add a mapping for a private identifier, which prints as `#` followed by `name`.
+    ///
+    /// The token is `#name`, so that is what the mapping is compared against and what it records.
+    pub fn add_source_mapping_for_private_name(&mut self, output: &[u8], span: Span, name: &str) {
+        let original_name = self.original_name(span, name);
+        let unchanged = original_name.is_some_and(|original| {
+            original
+                .as_bytes()
+                .split_first()
+                .is_some_and(|(&hash, rest)| hash == b'#' && rest == name.as_bytes())
+        });
+        let token_name = if unchanged { None } else { original_name };
+        self.add_source_mapping(output, span.start, token_name);
+    }
+
+    /// Get the source text `span` covers, which is the name to record unless nothing was renamed.
+    fn original_name(&self, span: Span, name: &str) -> Option<&'a str> {
         debug_assert!(
             (span.end as usize) <= self.original_source.len(),
             "violated {}:{} <= {} for {name}",
@@ -106,11 +130,7 @@ impl<'a> SourcemapBuilder<'a> {
             span.end,
             self.original_source.len()
         );
-        let original_name = self.original_source.get(span.start as usize..span.end as usize);
-        // The token name should be original name.
-        // If it hasn't change, name should be `None` to reduce `SourceMap` size.
-        let token_name = if original_name == Some(name) { None } else { original_name };
-        self.add_source_mapping(output, span.start, token_name);
+        self.original_source.get(span.start as usize..span.end as usize)
     }
 
     pub fn add_source_mapping(&mut self, output: &[u8], position: u32, name: Option<&'a str>) {
@@ -638,6 +658,61 @@ mod test {
         );
     }
 
+    #[test]
+    fn add_source_mapping_for_name_from_private_identifier() {
+        // A transform can print a plain identifier where the source had a private one.
+        // The `#` is part of the region the mapping describes, so it is part of the name recorded.
+        let output = b"foo";
+        let mut builder = SourcemapBuilder::new(Path::new("x.js"), "#foo");
+        builder.add_source_mapping_for_name(output, Span::new(0, 4), "foo");
+        let sm = builder.into_sourcemap();
+        assert_eq!(
+            sm.get_source_view_token(0_u32)
+                .as_ref()
+                .and_then(oxc_sourcemap::SourceViewToken::get_name),
+            Some("#foo")
+        );
+    }
+
+    #[test]
+    fn add_source_mapping_for_private_name() {
+        let output = b"#a#c";
+        let mut builder = SourcemapBuilder::new(Path::new("x.js"), "#a#b");
+        builder.add_source_mapping_for_private_name(output, Span::new(0, 2), "a");
+        builder.add_source_mapping_for_private_name(output, Span::new(2, 4), "c");
+        let sm = builder.into_sourcemap();
+        // The name `#a` not change.
+        assert_eq!(
+            sm.get_source_view_token(0_u32)
+                .as_ref()
+                .and_then(oxc_sourcemap::SourceViewToken::get_name),
+            None
+        );
+        // The name `#b` -> `#c`, save `#b` to token, `#` included.
+        assert_eq!(
+            sm.get_source_view_token(1_u32)
+                .as_ref()
+                .and_then(oxc_sourcemap::SourceViewToken::get_name),
+            Some("#b")
+        );
+    }
+
+    #[test]
+    fn add_source_mapping_for_private_name_from_plain_identifier() {
+        // A transform can print a private identifier where the source had a plain one.
+        // The token still differs from the source text, so the original name is recorded.
+        let output = b"#foo";
+        let mut builder = SourcemapBuilder::new(Path::new("x.js"), "ffoo");
+        builder.add_source_mapping_for_private_name(output, Span::new(0, 4), "foo");
+        let sm = builder.into_sourcemap();
+        assert_eq!(
+            sm.get_source_view_token(0_u32)
+                .as_ref()
+                .and_then(oxc_sourcemap::SourceViewToken::get_name),
+            Some("ffoo")
+        );
+    }
+
     #[test]
     fn add_source_mapping_for_unordered_position() {
         let output = b"";

crates/oxc_codegen/tests/integration/sourcemap.rs

@@ -4,8 +4,11 @@ use cow_utils::CowUtils;
 use oxc_allocator::Allocator;
 use oxc_ast::ast::{Expression, Statement};
 use oxc_codegen::{Codegen, CodegenOptions};
+use oxc_index::IndexVec;
 use oxc_parser::Parser;
 use oxc_span::{SourceType, Span};
+use oxc_str::CompactStr;
+use rustc_hash::FxHashMap;
 
 use crate::tester::default_options;
 
@@ -470,6 +473,44 @@ fn codegen(code: &str) -> (String, String) {
     (ret.code, ret.map.unwrap().to_data_url())
 }
 
+/// Print `source_text` with every private member of the first class renamed as `mappings` says,
+/// and return the names its source map records.
+fn mangled_private_names(source_text: &str, mappings: &[(&str, &str)]) -> Vec<String> {
+    let allocator = Allocator::default();
+    let ret = Parser::new(&allocator, source_text, SourceType::mjs()).parse();
+    assert!(ret.diagnostics.is_empty(), "parse errors: {:?}", ret.diagnostics);
+
+    let mut class_mappings = IndexVec::new();
+    class_mappings.push(
+        mappings
+            .iter()
+            .map(|(from, to)| ((*from).to_string(), CompactStr::from(*to)))
+            .collect::<FxHashMap<_, _>>(),
+    );
+
+    Codegen::new()
+        .with_options(default_options())
+        .with_private_member_mappings(Some(class_mappings))
+        .build(&ret.program)
+        .map
+        .expect("sourcemap should be generated")
+        .get_names()
+        .map(ToString::to_string)
+        .collect()
+}
+
+#[test]
+fn private_identifier_is_named_only_when_mangled() {
+    let source_text = "class C { #ab; m() { return this.#ab; } }";
+
+    // Printed as spelled, so nothing to name
+    assert_eq!(mangled_private_names(source_text, &[]), Vec::<String>::new());
+
+    // The token is `#ab`, so that is the name recorded - not `ab`, which would describe a region
+    // of the output which includes the `#`
+    assert_eq!(mangled_private_names(source_text, &[("ab", "a")]), vec!["#ab".to_string()]);
+}
+
 fn execute_with_node(code: &str, sourcemap_url: &str) -> String {
     let cwd = env::current_dir().unwrap().join("input.js");
     let cwd = cwd.to_str().unwrap();

packages/codegen/src-js/print/write.ts

@@ -465,13 +465,19 @@ function recordSourceMapping(state: State, node: MappableNode, location: Locatio
     // * When `matchesSource === false`, nothing is settled yet.
     //   It could be a genuine rename, or could be a Unicode escape (`\u0061` printed as `a`),
     //   or a non-ASCII character after the name, which `isDefinitelyIdentifierBoundary` will not classify.
+    //
+    // A private identifier prints as `#` followed by its name, and its span covers the `#`, so the token is `#name`.
+    // That is what the source is compared against, and what gets recorded as the name.
     const printedName = node.name;
-    const nameEnd = start + printedName.length;
+    const hashLength = node.type === "PrivateIdentifier" ? 1 : 0;
+    const nameStart = start + hashLength;
+    const nameEnd = nameStart + printedName.length;
     const matchesSource =
       printedName.length > 0
       && end <= sourceText.length
       && nameEnd <= end
-      && sourceText.startsWith(printedName, start)
+      && (hashLength === 0 || sourceText.charCodeAt(start) === 35) /* # */
+      && sourceText.startsWith(printedName, nameStart)
       && (nameEnd === end || isDefinitelyIdentifierBoundary(sourceText.charCodeAt(nameEnd)));
 
     if (!matchesSource) {
@@ -481,7 +487,7 @@ function recordSourceMapping(state: State, node: MappableNode, location: Locatio
 
       // A transformed or hand-authored AST can carry ranges unrelated to `sourceText`.
       // Preserve the existing fallback in that case instead of recording an arbitrary source substring.
-      if (originalName !== printedName) {
+      if (originalName === undefined || !isSameToken(originalName, printedName, hashLength)) {
         (state.mapNames ??= []).push(
           state.mapPositions.length >> 1,
           originalName === undefined ? printedName : originalName,
@@ -493,6 +499,21 @@ function recordSourceMapping(state: State, node: MappableNode, location: Locatio
   state.mapPositions.push(state.output.length, sourceOffset);
 }
 
+/**
+ * Is `originalName` the same token as `printedName`, taking into account leading `#` if `hashLength === 1`.
+ */
+function isSameToken(originalName: string, printedName: string, hashLength: 0 | 1): boolean {
+  if (hashLength === 0) return originalName === printedName;
+
+  // Compare with 2 operations rather than `originalName === "#" + printedName`
+  // to avoid allocating a temporary string
+  return (
+    originalName.length === printedName.length + 1
+    && originalName.charCodeAt(0) === 35 /* # */
+    && originalName.endsWith(printedName)
+  );
+}
+
 /**
  * Recover the original identifier spelling from validated source offsets.
  */

packages/codegen/test/source-maps.test.ts

@@ -17,6 +17,7 @@ import { checkFixture, getEcmaScriptLineTable } from "./utils/common.ts";
 
 import type { Program } from "oxc-parser";
 import type { SourceMap } from "../dist/index.js";
+import type * as ESTree from "../../../npm/oxc-types/types.d.ts";
 
 // Same directory the benchmarks download their fixtures to. Whichever are already cached are used.
 // The inline fixtures below always run.
@@ -168,6 +169,68 @@ describe("Rust conformance", () => {
     expect(map?.names).toEqual(["ab"]);
   });
 
+  test("a private identifier is named only when it was renamed", () => {
+    const code = "class C { #ab; m() { return this.#ab; } }";
+    const program = parseProgram("private.js", code);
+
+    // Printed as spelled, so nothing to name
+    expect(
+      printSync(program, { sourcemap: true, sourceFilename: "private.js", sourceText: code }).map
+        ?.names,
+    ).toEqual([]);
+
+    // Rename it, as a mangler would - every occurrence
+    renamePrivateIdentifiers(program, "ab", "a");
+
+    const { code: printed, map } = printSync(program, {
+      sourcemap: true,
+      sourceFilename: "private.js",
+      sourceText: code,
+    });
+    // The token is `#ab`, so that is the name recorded - not `ab`, which would describe a region
+    // of the output which includes the `#`
+    expect(printed).toContain("this.#a;");
+    expect(map?.names).toEqual(["#ab"]);
+  });
+
+  test("a plain identifier printed where the source was private is named with the `#`", () => {
+    const code = "class C { #ab; m() { return this.#ab; } }";
+    const program = parseProgram("unprivate.js", code);
+    const member = memberExpressionInFirstMethod(program);
+
+    // Simulate a transform which replaces `this.#ab` with `this.ab`, keeping the offsets.
+    // The `#` is part of the region the mapping describes, so it is part of the name recorded.
+    const { start, end } = member.property;
+    member.property = { type: "Identifier", name: "ab", start, end };
+
+    const { code: printed, map } = printSync(program, {
+      sourcemap: true,
+      sourceFilename: "unprivate.js",
+      sourceText: code,
+    });
+    expect(printed).toContain("this.ab;");
+    expect(map?.names).toEqual(["#ab"]);
+  });
+
+  test("a private identifier printed where the source was plain is named without a `#`", () => {
+    const code = "class C { m() { return this.ffoo; } }";
+    const program = parseProgram("privatised.js", code);
+    const member = memberExpressionInFirstMethod(program);
+
+    // Simulate a transform which replaces `this.ffoo` with `this.#foo`, keeping the offsets.
+    // The source token had no `#`, so the name recorded is the whole of what was there.
+    const { start, end } = member.property;
+    member.property = { type: "PrivateIdentifier", name: "foo", start, end };
+
+    const { code: printed, map } = printSync(program, {
+      sourcemap: true,
+      sourceFilename: "privatised.js",
+      sourceText: code,
+    });
+    expect(printed).toContain("this.#foo;");
+    expect(map?.names).toEqual(["ffoo"]);
+  });
+
   test("handles transformed ASTs whose source locations move backwards", () => {
     const code = `const first = 1;\n${"\n".repeat(5000)}const second = 2;`;
     const program = parseProgram("reordered.js", code);
@@ -540,3 +603,45 @@ describe("indent option", () => {
     expect(insideIndent).toBeNull();
   });
 });
+
+/**
+ * Rename every private identifier called `from` to `to`, as a mangler would.
+ */
+function renamePrivateIdentifiers(node: unknown, from: string, to: string): void {
+  if (node === null || typeof node !== "object") return;
+  if (Array.isArray(node)) {
+    for (const child of node) renamePrivateIdentifiers(child, from, to);
+    return;
+  }
+
+  const record = node as Record<string, unknown>;
+  if (record.type === "PrivateIdentifier" && record.name === from) record.name = to;
+  for (const key of Object.keys(record)) renamePrivateIdentifiers(record[key], from, to);
+}
+
+/**
+ * A static member expression whose property may be rewritten between its two forms.
+ *
+ * The AST types split the two apart, so neither of them alone types an assignment which swaps one
+ * for the other.
+ */
+type RewritableMemberExpression = Omit<ESTree.StaticMemberExpression, "property"> & {
+  property: ESTree.IdentifierName | ESTree.PrivateIdentifier;
+};
+
+/**
+ * The member expression a class's first method returns, for tests which rewrite its property.
+ */
+function memberExpressionInFirstMethod(program: Program): RewritableMemberExpression {
+  const statement = program.body[0];
+  if (statement.type !== "ClassDeclaration") throw new Error("Expected class declaration");
+  const method = statement.body.body.find((element) => element.type === "MethodDefinition");
+  if (method?.type !== "MethodDefinition") throw new Error("Expected method");
+  const returned = method.value.body?.body[0];
+  if (returned?.type !== "ReturnStatement") throw new Error("Expected return statement");
+  const member = returned.argument;
+  if (member?.type !== "MemberExpression" || member.computed) {
+    throw new Error("Expected static member expression");
+  }
+  return member as RewritableMemberExpression;
+}