fix(swiftpm): fix two ways array build settings were mishandled (#57744)

Summary:
Two bugs in `addArrayStringValues`, which adds members to an array build setting (`HEADER_SEARCH_PATHS`, `OTHER_LDFLAGS`, `FRAMEWORK_SEARCH_PATHS`, `LD_RUNPATH_SEARCH_PATHS`). Both hit real projects; neither was visible from the existing fixture.

**1. A promoted scalar was never restored.** A setting that already exists as a scalar gets promoted to a `( … )` array, but that was recorded as a plain member-append — so `deinit` stripped the members and left the array shell plus its injected `"$(inherited)"` behind. Stock Xcode projects hit this: the app template sets `LD_RUNPATH_SEARCH_PATHS = "$(inherited) executable_path/Frameworks";` as a target-level scalar.

Fixed by pinning the pre-injection value in `.spm-injected.json` and restoring it in place. It is stored raw (a bare scalar's token runs to the `;`, carrying whitespace that must come back), recorded only if the merge actually changed the field, and not restored if the field is gone.

**2. A one-line array was corrupted.** The append anchored on `lastIndexOf('\n', tokenEnd - 1)`, which assumes multi-line. With no newline in the value that lands on the *previous* line, so members were spliced above the field, outside the array:

```
{
		"/new",                             ← bare entry in the dict body: invalid pbxproj
	HEADER_SEARCH_PATHS = ("/vendor", );    ← member never added
}
```

The result is a project Xcode cannot open, and `deinit` could not remove the stray line. Xcode writes multi-line arrays, but hand-edited projects and other generators (XcodeGen, Tuist) emit compact ones. Fixed by splicing inline ahead of the `)`; removal gained matching delimiter-anchored patterns, so the span removed is the span inserted. The dedupe parse was also quote-blind — a member holding a quoted comma parsed as two tokens — and is now quote-aware.

**Tradeoff:** reversing a promotion rewrites the whole field, so members hand-added to a promoted array afterwards are lost.

**Rebase note:** `main` has since grown an overlapping guard (`buildSettingValueTokens`) that skips the append when every value is already present, avoiding the *no-op* promotion. It is kept and complements this change: main still promotes irreversibly when there *is* a fresh value to add to a scalar, which is what the restore here covers. The two records stay mutually exclusive per key, pinned by a test.

## Changelog:

[Internal] [Fixed] - SwiftPM: `spm deinit` restores a promoted scalar build setting, and `spm add` no longer corrupts a one-line array

Pull Request resolved: #57744

Test Plan:
`yarn jest packages/react-native/scripts` → **963 tests, 32 suites** green; eslint, prettier and flow clean.

Written red first: byte-identical `add` → `deinit` round-trips for each pre-existing shape (absent, multi-line, bare and quoted scalars, and the one-line forms), plus `add` → `update` → `deinit`. The multi-line path is unchanged byte-for-byte, verified by a differential harness over 48 add/remove cases against the previous implementation.

Re-verified after the rebase on the committed `HelloWorld.xcodeproj`, driving the real `injectSpmIntoExistingXcodeproj` / `removeSpmInjection`. On `main` a one-line `HEADER_SEARCH_PATHS` gains a bare `"…/autolinking/headers",` entry above the field and never receives the member; with this change it lands inside the array, and a pre-existing scalar comes back exactly.

Reviewed By: fabriziocucci

Differential Revision: D114317839

Pulled By: cipolleschi

fbshipit-source-id: 08bc81f80855721bf2123143063462109a99ac25

Author: chrfalch

packages/react-native/scripts/spm/__doc__/spm-scripts.md

@@ -222,7 +222,7 @@ left alone.
 | Path | Commit? | Why |
 |------|---------|-----|
 | `MyApp.xcodeproj/` | Yes | Your project, with SwiftPM injected in place. Holds your signing, capabilities, Build Phases — `add` only adds SwiftPM refs/settings, additively. |
-| `MyApp.xcodeproj/.spm-injected.json` | Yes | Marker recording every edit `add` made, so `deinit` can surgically reverse it and re-runs stay idempotent. Also pins settings later runs and Xcode builds must reuse: the `--version` pin (`artifactsVersionOverride`) that keeps later runs on the same artifact slots, and the [autolinking config command](#the-autolinking-config-command-is-remembered). |
+| `MyApp.xcodeproj/.spm-injected.json` | Yes | Marker recording every edit `add` made — plus the pre-injection value of any build setting it rewrote — so `deinit` can surgically reverse it and re-runs stay idempotent. Also pins settings later runs and Xcode builds must reuse: the `--version` pin (`artifactsVersionOverride`) that keeps later runs on the same artifact slots, and the [autolinking config command](#the-autolinking-config-command-is-remembered). |
 | `build/generated/` | No | Codegen/autolinking output; regenerated |
 | `build/xcframeworks/` | No | Symlinks to the machine-local artifact cache |
 | `Package.resolved` | No | SwiftPM resolution file; machine-specific |
@@ -234,7 +234,22 @@ stays untouched, and a re-run is a no-op. The injected refs point at three
 stable sub-package paths under `build/`; adding or removing community deps
 changes the sub-package contents (gitignored) and never re-injects. `deinit`
 removes exactly what was injected (using the marker), leaving the project
-byte-identical to its pre-`add` state.
+byte-identical to its pre-`add` state — with one exception, described next.
+
+**Build settings that already exist** are edited in place. The four array
+settings `add` merges into — `HEADER_SEARCH_PATHS`, `OTHER_LDFLAGS`,
+`FRAMEWORK_SEARCH_PATHS`, `LD_RUNPATH_SEARCH_PATHS` — keep the shape they were
+written in: Xcode's multi-line form as well as the compact one-line form hand
+edits and other generators (XcodeGen, Tuist) emit. One that exists as a plain
+*scalar* is promoted to a `( … )` array — the shape an Xcode-authored target can
+carry, e.g. a
+`LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks";` written
+as a scalar rather than a list. `add` records the pre-injection value in the
+marker and
+`deinit` restores it by rewriting the whole field — once folded together, the
+injected members and your own are indistinguishable — so **members you add to
+a promoted array by hand afterwards are lost**. That applies to `update` too,
+which reverts to the recorded baseline before re-injecting.
 
 Because everything under `build/` is gitignored, a clean checkout has no
 resolvable Swift packages until they are regenerated — see the next section.

packages/react-native/scripts/spm/__tests__/inject-spm-xcodeproj-test.js

@@ -56,6 +56,15 @@ function buildSettingsOf(text, configUuid) {
   );
   return text.slice(open, text.indexOf('};', open));
 }
+// Derive a variant whose app-target configs already carry HEADER_SEARCH_PATHS,
+// set to any valid pbxproj value: a plain scalar (which injection promotes to an
+// array) or an array injection appends to.
+function withHeaderSearchPaths(value) {
+  return PLAIN.replaceAll(
+    'PRODUCT_BUNDLE_IDENTIFIER = com.example.MyApp;',
+    `HEADER_SEARCH_PATHS = ${value};\n\t\t\t\tPRODUCT_BUNDLE_IDENTIFIER = com.example.MyApp;`,
+  );
+}
 
 const RN_PATH = '../node_modules/react-native';
 
@@ -289,6 +298,50 @@ describe('injectSpmIntoPbxproj — Tier 2 (build settings + phase)', () => {
     expect(syncIdx).toBeLessThan(sourcesIdx);
   });
 
+  it.each([
+    [
+      '"$(inherited)"',
+      ['"$(inherited)"', '"$(SRCROOT)/build/generated/autolinking/headers"'],
+    ],
+    [
+      '"$(inherited) $(SRCROOT)/vendor/include"',
+      [
+        '"$(inherited)"',
+        '"$(inherited) $(SRCROOT)/vendor/include"',
+        '"$(SRCROOT)/build/generated/autolinking/headers"',
+      ],
+    ],
+  ])(
+    'promotes a pre-existing HEADER_SEARCH_PATHS scalar (%s) to an array, keeping its value and one $(inherited)',
+    (scalar, expectedMembers) => {
+      const {text} = inject(withHeaderSearchPaths(scalar));
+      const arrays = [
+        ...text.matchAll(/HEADER_SEARCH_PATHS = \(\n([\s\S]*?)\t+\);/g),
+      ].map(m =>
+        m[1]
+          .split('\n')
+          .map(line => line.trim().replace(/,$/, ''))
+          .filter(member => member.length > 0),
+      );
+      // Both app-target configs (Debug + Release).
+      expect(arrays).toEqual([expectedMembers, expectedMembers]);
+    },
+  );
+
+  it('appends to a pre-existing ONE-LINE HEADER_SEARCH_PATHS array in place', () => {
+    const {text} = inject(withHeaderSearchPaths('("$(inherited)", )'));
+    expect(isBalanced(text)).toBe(true);
+    const arrays = [
+      ...text.matchAll(/HEADER_SEARCH_PATHS = \(([^\n]*)\);/g),
+    ].map(m => m[1]);
+    // Both app-target configs, each keeping the one-line shape it was written in.
+    expect(arrays).toEqual(
+      Array(2).fill(
+        '"$(inherited)", "$(SRCROOT)/build/generated/autolinking/headers", ',
+      ),
+    );
+  });
+
   it('adds one generated embed phase immediately after Frameworks', () => {
     const {text} = inject(PLAIN);
     expect(text).not.toContain('Fix SPM Embedded Flavor');

packages/react-native/scripts/spm/__tests__/remove-spm-injection-test.js

@@ -39,12 +39,41 @@ afterEach(() => {
 // Build a throwaway app dir: <tmp>/MyApp.xcodeproj/project.pbxproj seeded with
 // the plain (SPM-only) fixture, and a node_modules/react-native sibling so the
 // relative reactNativePath resolves.
-function scaffoldApp() {
+// Pre-existing values for an injected array setting (HEADER_SEARCH_PATHS),
+// seeded into both app-target configs. The plain fixture has none, so it only
+// ever exercises the create-from-absent path; deinit must restore each of
+// these forms — a plain scalar is ordinary, valid pbxproj.
+const PRE_EXISTING_HEADER_SEARCH_PATHS = {
+  'a bare $(inherited) scalar': '"$(inherited)"',
+  'a scalar with real content': '"$(inherited) $(SRCROOT)/vendor/include"',
+  'an array': '(\n\t\t\t\t"$(inherited)",\n\t\t\t)',
+  // What hand edits and other generators (XcodeGen, Tuist) write.
+  'a one-line array': '("$(inherited)", )',
+};
+
+// Seed a whole `KEY = value;` field (comments and stray whitespace included)
+// into both app-target configs.
+function withSetting(field /*: string */) {
+  return PLAIN.replaceAll(
+    'PRODUCT_BUNDLE_IDENTIFIER = com.example.MyApp;',
+    `${field}\n\t\t\t\tPRODUCT_BUNDLE_IDENTIFIER = com.example.MyApp;`,
+  );
+}
+
+function withHeaderSearchPaths(value /*: string */) {
+  return withSetting(`HEADER_SEARCH_PATHS = ${value};`);
+}
+
+function scaffoldApp(pbxproj /*: string */ = PLAIN) {
   const appRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'spm-deinit-'));
   scaffoldedAppRoots.push(appRoot);
   const xcodeprojPath = path.join(appRoot, 'MyApp.xcodeproj');
   fs.mkdirSync(xcodeprojPath, {recursive: true});
-  fs.writeFileSync(path.join(xcodeprojPath, 'project.pbxproj'), PLAIN, 'utf8');
+  fs.writeFileSync(
+    path.join(xcodeprojPath, 'project.pbxproj'),
+    pbxproj,
+    'utf8',
+  );
   const rnRoot = path.join(appRoot, 'node_modules', 'react-native');
   fs.mkdirSync(rnRoot, {recursive: true});
   const artifactRoot = path.join(appRoot, 'build', 'xcframeworks');
@@ -1179,3 +1208,172 @@ describe('readPinnedConfigCommand', () => {
     expect(readPinnedConfigCommand(appRoot)).toBeNull();
   });
 });
+
+describe.each(Object.entries(PRE_EXISTING_HEADER_SEARCH_PATHS))(
+  'removeSpmInjection with HEADER_SEARCH_PATHS already set to %s',
+  (_label, value) => {
+    it('restores the pre-existing value byte-for-byte', () => {
+      const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp(
+        withHeaderSearchPaths(value),
+      );
+      const before = pbxprojOf(xcodeprojPath);
+
+      injectSpmIntoExistingXcodeproj({
+        appRoot,
+        reactNativeRoot: rnRoot,
+        xcodeprojPath,
+      });
+      expect(pbxprojOf(xcodeprojPath)).not.toBe(before);
+
+      expect(removeSpmInjection({appRoot, xcodeprojPath}).status).toBe(
+        'removed',
+      );
+      expect(pbxprojOf(xcodeprojPath)).toBe(before);
+    });
+
+    it('re-syncing is byte-for-byte identical', () => {
+      const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp(
+        withHeaderSearchPaths(value),
+      );
+      injectSpmIntoExistingXcodeproj({
+        appRoot,
+        reactNativeRoot: rnRoot,
+        xcodeprojPath,
+      });
+      const first = pbxprojOf(xcodeprojPath);
+      injectSpmIntoExistingXcodeproj({
+        appRoot,
+        reactNativeRoot: rnRoot,
+        xcodeprojPath,
+      });
+      expect(pbxprojOf(xcodeprojPath)).toBe(first);
+    });
+  },
+);
+
+// findField's token for a BARE scalar ends AT the `;`, so it includes any
+// whitespace before it. Deinit must put those bytes back exactly, not a
+// tidied-up version of them.
+describe.each([
+  'HEADER_SEARCH_PATHS = $(inherited)   ; /* note */',
+  'HEADER_SEARCH_PATHS =   ;',
+])('removeSpmInjection with the untrimmed scalar `%s`', field => {
+  it('restores it byte-for-byte', () => {
+    const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp(withSetting(field));
+    const before = pbxprojOf(xcodeprojPath);
+
+    injectSpmIntoExistingXcodeproj({
+      appRoot,
+      reactNativeRoot: rnRoot,
+      xcodeprojPath,
+    });
+    expect(pbxprojOf(xcodeprojPath)).not.toBe(before);
+
+    expect(removeSpmInjection({appRoot, xcodeprojPath}).status).toBe('removed');
+    expect(pbxprojOf(xcodeprojPath)).toBe(before);
+  });
+});
+
+describe('a scalar array setting injection has nothing to add to', () => {
+  const SCALAR = 'FRAMEWORK_SEARCH_PATHS = "$(inherited)";';
+  const EDITED = 'FRAMEWORK_SEARCH_PATHS = "$(inherited) $(SRCROOT)/Vendor";';
+
+  // The fixture's flavored-frameworks manifest is empty, so
+  // FRAMEWORK_SEARCH_PATHS is injected with no values at all.
+  it('is left untouched, unrecorded, and survives a later user edit', () => {
+    const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp(withSetting(SCALAR));
+
+    injectSpmIntoExistingXcodeproj({
+      appRoot,
+      reactNativeRoot: rnRoot,
+      xcodeprojPath,
+    });
+
+    const injected = pbxprojOf(xcodeprojPath);
+    expect(injected).toContain(SCALAR);
+    expect(injected).not.toMatch(/FRAMEWORK_SEARCH_PATHS = \(/);
+    for (const change of readMarker(xcodeprojPath).buildSettingChanges) {
+      expect(change.promotedArrayScalars ?? {}).not.toHaveProperty(
+        'FRAMEWORK_SEARCH_PATHS',
+      );
+    }
+
+    fs.writeFileSync(
+      path.join(xcodeprojPath, 'project.pbxproj'),
+      injected.replaceAll(SCALAR, EDITED),
+      'utf8',
+    );
+    removeSpmInjection({appRoot, xcodeprojPath});
+
+    const after = pbxprojOf(xcodeprojPath);
+    expect(after).toContain(EDITED);
+    expect(after).not.toContain(SCALAR);
+  });
+});
+
+describe('a promoted array setting the user deleted after add', () => {
+  const SCALAR = '"$(inherited) $(SRCROOT)/vendor/include"';
+
+  it('is not resurrected by deinit', () => {
+    const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp(
+      withHeaderSearchPaths(SCALAR),
+    );
+    const before = pbxprojOf(xcodeprojPath);
+
+    injectSpmIntoExistingXcodeproj({
+      appRoot,
+      reactNativeRoot: rnRoot,
+      xcodeprojPath,
+    });
+
+    const deleted = pbxprojOf(xcodeprojPath).replace(
+      /\n\t+HEADER_SEARCH_PATHS = \(\n[\s\S]*?\n\t+\);/g,
+      '',
+    );
+    expect(deleted).not.toContain('HEADER_SEARCH_PATHS');
+    fs.writeFileSync(
+      path.join(xcodeprojPath, 'project.pbxproj'),
+      deleted,
+      'utf8',
+    );
+
+    removeSpmInjection({appRoot, xcodeprojPath});
+
+    // Everything else is back to its pre-injection bytes; only the setting the
+    // user deleted stays gone.
+    expect(pbxprojOf(xcodeprojPath)).toBe(
+      before.replaceAll(`\n\t\t\t\tHEADER_SEARCH_PATHS = ${SCALAR};`, ''),
+    );
+  });
+});
+
+// `deinit` removes appendedArrayValues before it restores promotedArrayScalars,
+// so recording a key under both happens to come out right today: the scalar
+// restore rewrites the whole value last. That makes the exclusivity below
+// invisible to a round-trip test, which is why it is asserted on the marker
+// directly — reversing those two loops would otherwise silently start removing
+// array members from an already-restored scalar.
+describe('a promoted scalar is recorded once, not twice', () => {
+  it('records promotedArrayScalars and not appendedArrayValues for the key', () => {
+    const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp(
+      withHeaderSearchPaths('"$(inherited) $(SRCROOT)/vendor/include"'),
+    );
+
+    injectSpmIntoExistingXcodeproj({
+      appRoot,
+      reactNativeRoot: rnRoot,
+      xcodeprojPath,
+    });
+
+    const changes = readMarker(xcodeprojPath).buildSettingChanges;
+    expect(changes.length).toBeGreaterThan(0);
+    for (const change of changes) {
+      expect(Object.keys(change.promotedArrayScalars ?? {})).toContain(
+        'HEADER_SEARCH_PATHS',
+      );
+      expect(Object.keys(change.appendedArrayValues ?? {})).not.toContain(
+        'HEADER_SEARCH_PATHS',
+      );
+    }
+  });
+});