Skip to content

Commit f4c790d

Browse files
chrfalchclaude
andcommitted
SPM: stop corrupting a one-line array build setting
addArrayStringValues' "already an array" branch assumed the array was multi-line: it anchored on `lastIndexOf('\n', tokenEnd - 1)`. For a single-line array there is no newline inside the value, so that lands at the end of the PREVIOUS line and the new members were spliced above the field — outside the array, as a bare entry in the dict body: { "/new", <- invalid pbxproj HEADER_SEARCH_PATHS = ("/vendor", ); <- member never added OTHER = 1; } One `spm add` against such a project produced a file Xcode cannot open, and because removeArrayStringValues only searches inside the field's value region, `deinit` could never remove the stray line. Xcode writes multi-line arrays, but hand-edited projects and other generators (XcodeGen, Tuist) emit compact ones. Splice the members inline ahead of the `)` instead, matching the separator style already present and honouring an existing trailing comma. Reformatting to multi-line would change the user's formatting and would need the old shape recorded to stay reversible, for no benefit. removeArrayStringValues gains delimiter-anchored patterns for the shapes `add` can now produce, so the span removed is exactly the span inserted and every shape round-trips byte-for-byte. The dedupe parse was also quote-blind: it split members on every `,`, so a member holding a quoted comma (`"$(FOO(x)),weird"`) parsed as two tokens and defeated the exact-token short-circuit. It now uses a quote-aware splitter. The multi-line path is unchanged byte-for-byte, verified by a differential harness against the previous implementation over 48 add/remove cases, plus a test pinning its exact output bytes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 4ff8782 commit f4c790d

4 files changed

Lines changed: 223 additions & 26 deletions

File tree

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

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,10 @@ const PODS = PLAIN.replace(
2929
'AA0000000000000000000901 /* Debug */ = {\n\t\t\tisa = XCBuildConfiguration;\n\t\t\tbaseConfigurationReference = BB0000000000000000000001 /* Pods-MyApp.debug.xcconfig */;\n\t\t\tbuildSettings = {',
3030
);
3131

32-
// Derive a variant whose app-target configs already carry HEADER_SEARCH_PATHS
33-
// as a plain scalar (ordinary, valid pbxproj) — the state injection promotes to
34-
// an array.
35-
function withScalarHeaderSearchPaths(value) {
32+
// Derive a variant whose app-target configs already carry HEADER_SEARCH_PATHS,
33+
// set to any valid pbxproj value: a plain scalar (which injection promotes to an
34+
// array) or an array injection appends to.
35+
function withHeaderSearchPaths(value) {
3636
return PLAIN.replaceAll(
3737
'PRODUCT_BUNDLE_IDENTIFIER = com.example.MyApp;',
3838
`HEADER_SEARCH_PATHS = ${value};\n\t\t\t\tPRODUCT_BUNDLE_IDENTIFIER = com.example.MyApp;`,
@@ -253,7 +253,7 @@ describe('injectSpmIntoPbxproj — Tier 2 (build settings + phase)', () => {
253253
])(
254254
'promotes a pre-existing HEADER_SEARCH_PATHS scalar (%s) to an array, keeping its value and one $(inherited)',
255255
(scalar, expectedMembers) => {
256-
const {text} = inject(withScalarHeaderSearchPaths(scalar));
256+
const {text} = inject(withHeaderSearchPaths(scalar));
257257
const arrays = [
258258
...text.matchAll(/HEADER_SEARCH_PATHS = \(\n([\s\S]*?)\t+\);/g),
259259
].map(m =>
@@ -267,6 +267,20 @@ describe('injectSpmIntoPbxproj — Tier 2 (build settings + phase)', () => {
267267
},
268268
);
269269

270+
it('appends to a pre-existing ONE-LINE HEADER_SEARCH_PATHS array in place', () => {
271+
const {text} = inject(withHeaderSearchPaths('("$(inherited)", )'));
272+
expect(isBalanced(text)).toBe(true);
273+
const arrays = [
274+
...text.matchAll(/HEADER_SEARCH_PATHS = \(([^\n]*)\);/g),
275+
].map(m => m[1]);
276+
// Both app-target configs, each keeping the one-line shape it was written in.
277+
expect(arrays).toEqual(
278+
Array(2).fill(
279+
'"$(inherited)", "$(SRCROOT)/build/generated/autolinking/headers", ',
280+
),
281+
);
282+
});
283+
270284
it('adds one generated embed phase immediately after Frameworks', () => {
271285
const {text} = inject(PLAIN);
272286
expect(text).not.toContain('Fix SPM Embedded Flavor');

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ const PRE_EXISTING_HEADER_SEARCH_PATHS = {
4242
'a bare $(inherited) scalar': '"$(inherited)"',
4343
'a scalar with real content': '"$(inherited) $(SRCROOT)/vendor/include"',
4444
'an array': '(\n\t\t\t\t"$(inherited)",\n\t\t\t)',
45+
// What hand edits and other generators (XcodeGen, Tuist) write.
46+
'a one-line array': '("$(inherited)", )',
4547
};
4648

4749
// Seed a whole `KEY = value;` field (comments and stray whitespace included)

packages/react-native/scripts/spm/__tests__/spm-pbxproj-test.js

Lines changed: 109 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,19 @@ const PLAIN_PBXPROJ = fs.readFileSync(
4141
'utf8',
4242
);
4343

44+
// The app target's Debug buildSettings dict, as a body range.
45+
function targetDebugDict(text) {
46+
const cfg = findObjectByUuid(text, 'AA0000000000000000000901');
47+
const bs = findField(text, cfg, 'buildSettings');
48+
return {uuid: 'x', bodyOpen: bs.valueStart, bodyClose: bs.tokenEnd - 1};
49+
}
50+
51+
// Delimiter balance, checked with the module's own quote-aware scanner: the
52+
// outermost `{` must close on the file's last `}`.
53+
function isBalanced(text) {
54+
return scanToClose(text, text.indexOf('{')) === text.lastIndexOf('}');
55+
}
56+
4457
// ---------------------------------------------------------------------------
4558
// generateUUID
4659
// ---------------------------------------------------------------------------
@@ -161,12 +174,6 @@ describe('addArrayMembers', () => {
161174
});
162175

163176
describe('addArrayStringValues', () => {
164-
function targetDebugDict(text) {
165-
const cfg = findObjectByUuid(text, 'AA0000000000000000000901');
166-
const bs = findField(text, cfg, 'buildSettings');
167-
return {uuid: 'x', bodyOpen: bs.valueStart, bodyClose: bs.tokenEnd - 1};
168-
}
169-
170177
it('creates an array seeded with $(inherited)', () => {
171178
const out = addArrayStringValues(
172179
PLAIN_PBXPROJ,
@@ -270,6 +277,102 @@ describe('addArrayStringValues', () => {
270277
});
271278
});
272279

280+
// Xcode writes array build settings multi-line, but hand-edited projects and
281+
// other generators (XcodeGen, Tuist) emit compact one-line ones. Members must
282+
// land INSIDE the array whatever its shape, and `deinit` must be able to take
283+
// them back out again — hence the byte-identical add→remove round trip.
284+
describe('array build settings of every written shape', () => {
285+
const NEW = '"/new"';
286+
287+
// Seed `OTHER_LDFLAGS = <value>;` into the app target's Debug config.
288+
function withValue(value) {
289+
return PLAIN_PBXPROJ.replace(
290+
'\t\t\t\tPRODUCT_NAME = "$(TARGET_NAME)";',
291+
`\t\t\t\tOTHER_LDFLAGS = ${value};\n\t\t\t\tPRODUCT_NAME = "$(TARGET_NAME)";`,
292+
);
293+
}
294+
295+
function add(text, values) {
296+
return addArrayStringValues(
297+
text,
298+
targetDebugDict(text),
299+
'OTHER_LDFLAGS',
300+
values,
301+
);
302+
}
303+
304+
function remove(text, values) {
305+
return removeArrayStringValues(
306+
text,
307+
targetDebugDict(text),
308+
'OTHER_LDFLAGS',
309+
values,
310+
);
311+
}
312+
313+
describe.each([
314+
['an empty array', '()'],
315+
['a lone member with no trailing comma', '("/a")'],
316+
['a trailing comma and space', '("/a", )'],
317+
['no space after the comma', '("/a","/b")'],
318+
['a space after the comma', '("/a", "/b")'],
319+
['a member whose quotes hold a comma and parens', '("$(FOO(x)),weird")'],
320+
['the multi-line shape Xcode writes', '(\n\t\t\t\t\t"/a",\n\t\t\t\t)'],
321+
])('%s', (_label, shape) => {
322+
const input = withValue(shape);
323+
324+
it('adds the value inside the array, leaving the file balanced', () => {
325+
const out = add(input, [NEW]);
326+
const field = findField(out, targetDebugDict(out), 'OTHER_LDFLAGS');
327+
expect(field.value.trimStart().startsWith('(')).toBe(true);
328+
expect(field.value).toContain(NEW);
329+
// Nothing was spliced ahead of the field — i.e. outside the array.
330+
expect(out.slice(0, field.matchStart)).toBe(
331+
input.slice(0, field.matchStart),
332+
);
333+
expect(isBalanced(out)).toBe(true);
334+
});
335+
336+
it.each([[[NEW]], [[NEW, '"/new2"']]])(
337+
'remove undoes add of %j byte-for-byte',
338+
values => {
339+
const added = add(input, values);
340+
expect(added).not.toBe(input);
341+
expect(remove(added, values)).toBe(input);
342+
},
343+
);
344+
});
345+
346+
it.each([
347+
['a value that is not there', '("/a", )', '"/zzz"'],
348+
['a member stripped of its quotes', '("$(inherited)", )', '$(inherited)'],
349+
])('removes nothing when asked for %s', (_label, shape, value) => {
350+
const input = withValue(shape);
351+
expect(remove(input, [value])).toBe(input);
352+
});
353+
354+
it('is a no-op when a one-line array already holds the value', () => {
355+
const input = withValue(`(${NEW})`);
356+
expect(add(input, [NEW])).toBe(input);
357+
});
358+
359+
it('dedupes a member whose quotes hold a comma', () => {
360+
const weird = '"$(FOO(x)),weird"';
361+
const input = withValue(`(${weird}, )`);
362+
expect(add(input, [weird])).toBe(input);
363+
});
364+
365+
it('splices a multi-line array on its own line, before the closing `)`', () => {
366+
const input = withValue('(\n\t\t\t\t\t"/a",\n\t\t\t\t)');
367+
expect(add(input, [NEW])).toBe(
368+
input.replace(
369+
'\t\t\t\t\t"/a",\n',
370+
`\t\t\t\t\t"/a",\n\t\t\t\t\t${NEW},\n`,
371+
),
372+
);
373+
});
374+
});
375+
273376
describe('ensureScalarField', () => {
274377
it('adds a scalar only when absent', () => {
275378
const project = findProjectObject(PLAIN_PBXPROJ);

packages/react-native/scripts/spm/spm-pbxproj.js

Lines changed: 93 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,38 @@ function addArrayMembers(
367367
return text.slice(0, obj.bodyOpen + 1) + block + text.slice(obj.bodyOpen + 1);
368368
}
369369

370+
/**
371+
* Indices of the `,` separators at an array's top level — a comma inside a
372+
* quoted member (`"$(FOO),weird"`) separates nothing.
373+
*/
374+
function topLevelCommas(inner /*: string */) /*: Array<number> */ {
375+
const out = [];
376+
for (let i = 0; i < inner.length; i++) {
377+
if (inner[i] === '"') {
378+
i = scanString(inner, i);
379+
} else if (inner[i] === ',') {
380+
out.push(i);
381+
}
382+
}
383+
return out;
384+
}
385+
386+
/**
387+
* The members of an array's inner text (what sits between its parens), trimmed
388+
* and with empty slots — e.g. the one a trailing comma leaves — dropped. A bare
389+
* scalar value parses as its own single member.
390+
*/
391+
function arrayMembers(inner /*: string */) /*: Array<string> */ {
392+
const members = [];
393+
let start = 0;
394+
for (const comma of topLevelCommas(inner)) {
395+
members.push(inner.slice(start, comma));
396+
start = comma + 1;
397+
}
398+
members.push(inner.slice(start));
399+
return members.map(m => m.trim()).filter(m => m !== '');
400+
}
401+
370402
/**
371403
* Append raw string values to a `( … )` array build-setting (e.g.
372404
* OTHER_LDFLAGS), deduping by exact token. Creates the setting seeded with
@@ -385,26 +417,45 @@ function addArrayStringValues(
385417

386418
const field = findField(text, obj, key);
387419
if (field != null) {
420+
const isArray = field.value.trimStart().startsWith('(');
421+
const openParen = isArray
422+
? field.valueStart + field.value.indexOf('(')
423+
: -1;
424+
const closeParen = isArray ? scanToClose(text, openParen) : -1;
425+
const inner = isArray ? text.slice(openParen + 1, closeParen) : field.value;
388426
// Dedup by EXACT existing member, not substring — a substring check would
389427
// treat `"-ObjC"` as already present when only `"-ObjCFoo"` is there (and
390-
// vice-versa). Parse the current members (array `( … )` or bare scalar).
391-
const existingMembers = new Set(
392-
field.value
393-
.replace(/^\s*\(/, '')
394-
.replace(/\)\s*$/, '')
395-
.split(',')
396-
.map(s => s.trim())
397-
.filter(s => s.length > 0),
398-
);
428+
// vice-versa).
429+
const existingMembers = new Set(arrayMembers(inner));
399430
const fresh = values.filter(v => !existingMembers.has(v));
400431
if (fresh.length === 0) {
401432
return text;
402433
}
403-
if (field.value.trimStart().startsWith('(')) {
404-
// Existing array — splice fresh members before the closing `)`.
405-
const lineStart = text.lastIndexOf('\n', field.tokenEnd - 1) + 1;
406-
const lines = fresh.map(v => `${memberIndent}${v},\n`).join('');
407-
return text.slice(0, lineStart) + lines + text.slice(lineStart);
434+
if (isArray) {
435+
if (inner.includes('\n')) {
436+
// Multi-line array — one member per line, before the closing `)`.
437+
const lineStart = text.lastIndexOf('\n', closeParen) + 1;
438+
const lines = fresh.map(v => `${memberIndent}${v},\n`).join('');
439+
return text.slice(0, lineStart) + lines + text.slice(lineStart);
440+
}
441+
// One-line array (hand-edited projects, XcodeGen, Tuist) — splice the
442+
// members in ahead of the `)`, in the separator style already there.
443+
// Reformatting it multi-line instead would have to record the old shape
444+
// to stay reversible on deinit.
445+
const commas = topLevelCommas(inner);
446+
const gapMatch =
447+
commas.length > 0 ? /^[\t ]*/.exec(inner.slice(commas[0] + 1)) : null;
448+
const gap = gapMatch != null ? gapMatch[0] : ' ';
449+
const core = inner.replace(/[\t ]+$/, '');
450+
const joined = fresh.join(`,${gap}`);
451+
const insertion =
452+
core === ''
453+
? joined
454+
: core.endsWith(',')
455+
? `${gap}${joined},`
456+
: `,${gap}${joined}`;
457+
const at = openParen + 1 + core.length;
458+
return text.slice(0, at) + insertion + text.slice(at);
408459
}
409460
// Existing scalar — promote to an array preserving the prior value. Skip it
410461
// when it IS the `"$(inherited)"` the array is seeded with (emitted twice),
@@ -537,6 +588,33 @@ function removeField(
537588
return text.slice(0, f.matchStart) + text.slice(f.tokenEnd + 1);
538589
}
539590

591+
/**
592+
* Drop one member from an array field's value text. The patterns mirror the
593+
* forms addArrayStringValues inserts, tried in the order that makes the span
594+
* removed exactly the span it added: a line of its own (multi-line array), then
595+
* after a comma, before a comma, or alone ahead of the `)` (the one-line
596+
* shapes). Each is anchored on a delimiter, so a value that is merely a prefix
597+
* of a longer member is never mistaken for it.
598+
*/
599+
function removeArrayMember(
600+
region /*: string */,
601+
value /*: string */,
602+
) /*: string */ {
603+
const v = escapeRegExp(value);
604+
for (const pattern of [
605+
`\\n[\\t ]*${v},`,
606+
`,[\\t ]*${v}(?=[\\t ]*[,)])`,
607+
`[\\t ]*${v},[\\t ]*`,
608+
`[\\t ]*${v}(?=[\\t ]*\\))`,
609+
]) {
610+
const shorter = region.replace(new RegExp(pattern), '');
611+
if (shorter !== region) {
612+
return shorter;
613+
}
614+
}
615+
return region;
616+
}
617+
540618
/**
541619
* Remove specific raw string members from an existing `( … )` array field
542620
* (inverse of addArrayStringValues' append branch). Leaves the field and any
@@ -554,7 +632,7 @@ function removeArrayStringValues(
554632
}
555633
let region = text.slice(f.valueStart, f.tokenEnd);
556634
for (const val of values) {
557-
region = region.replace(new RegExp(`\\n[\\t ]*${escapeRegExp(val)},`), '');
635+
region = removeArrayMember(region, val);
558636
}
559637
return text.slice(0, f.valueStart) + region + text.slice(f.tokenEnd);
560638
}

0 commit comments

Comments
 (0)