Skip to content

Commit 9b1a93f

Browse files
knutwannhedenknutwannheden
committed
JavaScript: Try to better do lenient type matching
1 parent 18814f7 commit 9b1a93f

2 files changed

Lines changed: 301 additions & 34 deletions

File tree

‎rewrite-javascript/rewrite/src/javascript/comparator.ts‎

Lines changed: 77 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1957,54 +1957,93 @@ export class JavaScriptSemanticComparatorVisitor extends JavaScriptComparatorVis
19571957

19581958
const otherMethod = other as J.MethodInvocation;
19591959

1960-
// Check basic structural equality first
1961-
if (method.name.simpleName !== otherMethod.name.simpleName ||
1962-
method.arguments.elements.length !== otherMethod.arguments.elements.length) {
1960+
// Check argument length (always required)
1961+
if (method.arguments.elements.length !== otherMethod.arguments.elements.length) {
19631962
return this.abort(method);
19641963
}
19651964

1966-
// Check type attribution
1967-
// Both must have method types for semantic equality
1968-
if (!method.methodType || !otherMethod.methodType) {
1969-
// Lenient mode: if either has no type, allow structural matching
1970-
if (this.lenientTypeMatching) {
1971-
return super.visitMethodInvocation(method, other);
1965+
// Check if we can skip name checking based on type attribution
1966+
// We can only skip the name check if both have method types AND they represent the SAME method
1967+
// (not just type-compatible methods, but the actual same function with same FQN)
1968+
let canSkipNameCheck = false;
1969+
if (method.methodType && otherMethod.methodType) {
1970+
// Check if both method types have fully qualified declaring types with the same FQN
1971+
// This indicates they're the same method from the same module (possibly aliased)
1972+
const methodDeclaringType = method.methodType.declaringType;
1973+
const otherDeclaringType = otherMethod.methodType.declaringType;
1974+
1975+
if (methodDeclaringType && otherDeclaringType &&
1976+
Type.isFullyQualified(methodDeclaringType) && Type.isFullyQualified(otherDeclaringType)) {
1977+
1978+
const methodFQN = Type.FullyQualified.getFullyQualifiedName(methodDeclaringType as Type.FullyQualified);
1979+
const otherFQN = Type.FullyQualified.getFullyQualifiedName(otherDeclaringType as Type.FullyQualified);
1980+
1981+
// Same module/class AND same method name in the type = same method (can be aliased)
1982+
if (methodFQN === otherFQN && method.methodType.name === otherMethod.methodType.name) {
1983+
canSkipNameCheck = true;
1984+
}
1985+
// If FQNs or method names don't match, we can't skip name check - fall through to name checking
19721986
}
1973-
// Strict mode: if one has type but the other doesn't, they don't match
1974-
if (method.methodType || otherMethod.methodType) {
1987+
// If one or both don't have fully qualified types, we can't safely skip name checking
1988+
// Fall through to normal name comparison below
1989+
}
1990+
1991+
// Check names unless we determined we can skip based on type FQN matching
1992+
if (!canSkipNameCheck) {
1993+
if (method.name.simpleName !== otherMethod.name.simpleName) {
19751994
return this.abort(method);
19761995
}
1977-
// If neither has type, fall through to structural comparison
1978-
return super.visitMethodInvocation(method, other);
1979-
}
19801996

1981-
// Both have types - check they match semantically
1982-
const typesMatch = this.isOfType(method.methodType, otherMethod.methodType);
1983-
if (!typesMatch) {
1984-
// Types don't match - abort comparison
1985-
return this.abort(method);
1986-
}
1997+
// In strict mode, check type attribution requirements
1998+
if (!this.lenientTypeMatching) {
1999+
// Strict mode: if one has type but the other doesn't, they don't match
2000+
if ((method.methodType && !otherMethod.methodType) ||
2001+
(!method.methodType && otherMethod.methodType)) {
2002+
return this.abort(method);
2003+
}
2004+
}
2005+
2006+
// If neither has type, use structural comparison
2007+
if (!method.methodType && !otherMethod.methodType) {
2008+
return super.visitMethodInvocation(method, other);
2009+
}
19872010

1988-
// Types match! Now check if we can ignore receiver differences.
1989-
// We can only ignore receiver differences when one or both receivers are identifiers
1990-
// that represent module/namespace imports (e.g., `util` in `util.isDate()`).
1991-
// For other receivers (e.g., variables, expressions), we must compare them.
2011+
// If both have types with FQ declaring types, verify they're compatible
2012+
// (This prevents matching completely different methods like util.isArray vs util.isBoolean)
2013+
if (method.methodType && otherMethod.methodType) {
2014+
const methodDeclaringType = method.methodType.declaringType;
2015+
const otherDeclaringType = otherMethod.methodType.declaringType;
19922016

1993-
const canIgnoreReceiverDifference =
1994-
// Case 1: One has no select (direct call like `forwardRef()`), other has select (namespace like `React.forwardRef()`)
1995-
(!method.select && otherMethod.select) ||
1996-
(method.select && !otherMethod.select);
2017+
if (methodDeclaringType && otherDeclaringType &&
2018+
Type.isFullyQualified(methodDeclaringType) && Type.isFullyQualified(otherDeclaringType)) {
19972019

1998-
if (!canIgnoreReceiverDifference) {
1999-
// Both have selects or both don't - must compare them structurally
2020+
const methodFQN = Type.FullyQualified.getFullyQualifiedName(methodDeclaringType as Type.FullyQualified);
2021+
const otherFQN = Type.FullyQualified.getFullyQualifiedName(otherDeclaringType as Type.FullyQualified);
2022+
2023+
// Different declaring types = different methods, even with same name
2024+
if (methodFQN !== otherFQN) {
2025+
return this.abort(method);
2026+
}
2027+
}
2028+
}
2029+
}
2030+
2031+
// When types match (canSkipNameCheck = true), we can skip select comparison entirely.
2032+
// This allows matching forwardRef() vs React.forwardRef() where types indicate same method.
2033+
if (!canSkipNameCheck) {
2034+
// Types didn't provide a match - must compare receivers structurally
20002035
if ((method.select === undefined) !== (otherMethod.select === undefined)) {
20012036
return this.abort(method);
20022037
}
20032038

20042039
if (method.select && otherMethod.select) {
20052040
await this.visitRightPadded(method.select, otherMethod.select as any);
2041+
if (!this.match) {
2042+
return this.abort(method);
2043+
}
20062044
}
20072045
}
2046+
// else: types matched, skip select comparison (allows namespace vs named imports)
20082047

20092048
// Compare type parameters
20102049
if ((method.typeParameters === undefined) !== (otherMethod.typeParameters === undefined)) {
@@ -2023,10 +2062,14 @@ export class JavaScriptSemanticComparatorVisitor extends JavaScriptComparatorVis
20232062
}
20242063
}
20252064

2026-
// Compare name (already checked simpleName above, but visit for markers/prefix)
2027-
await this.visit(method.name, otherMethod.name);
2028-
if (!this.match) {
2029-
return this.abort(method);
2065+
// Compare name
2066+
// If we determined we can skip name check (same FQN method, possibly aliased), skip it
2067+
// This allows matching aliased imports where names differ but types are the same
2068+
if (!canSkipNameCheck) {
2069+
await this.visit(method.name, otherMethod.name);
2070+
if (!this.match) {
2071+
return this.abort(method);
2072+
}
20302073
}
20312074

20322075
// Compare arguments (visit RightPadded to check for markers)

‎rewrite-javascript/rewrite/test/javascript/templating/lenient-type-matching.test.ts‎

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,4 +255,228 @@ function greet(): string { return "hello"; }
255255
expect(capturedName).toBeDefined();
256256
expect((capturedName as J.Identifier).simpleName).toBe('greet');
257257
});
258+
259+
test('strict mode with aliased import should match based on type', async () => {
260+
await withDir(async (repo) => {
261+
const tempDir = repo.path;
262+
263+
// Pattern uses the original import name
264+
const pat = pattern`isDate(${capture('arg')})`
265+
.configure({
266+
context: [`import { isDate } from 'node:util/types'`],
267+
lenientTypeMatching: false, // Strict type matching
268+
dependencies: {'@types/node': '^20.0.0'}
269+
});
270+
271+
let matchFound = false;
272+
let capturedArg: any = undefined;
273+
274+
spec.recipe = fromVisitor(new class extends JavaScriptVisitor<any> {
275+
override async visitMethodInvocation(methodInvocation: J.MethodInvocation, _p: any): Promise<J | undefined> {
276+
const m = await pat.match(methodInvocation);
277+
if (m) {
278+
matchFound = true;
279+
capturedArg = m.get('arg');
280+
}
281+
return methodInvocation;
282+
}
283+
});
284+
285+
await spec.rewriteRun(
286+
npm(
287+
tempDir,
288+
//language=typescript
289+
typescript(
290+
`
291+
import { isDate as isDateFn } from 'node:util/types';
292+
293+
const result = isDateFn(new Date());
294+
`
295+
),
296+
//language=json
297+
packageJson(
298+
`
299+
{
300+
"name": "test",
301+
"version": "1.0.0",
302+
"dependencies": {
303+
"@types/node": "^20.0.0"
304+
}
305+
}
306+
`
307+
)
308+
)
309+
);
310+
311+
// With strict type matching, aliased imports should still match if types match
312+
expect(matchFound).toBe(true);
313+
expect(capturedArg).toBeDefined();
314+
}, {unsafeCleanup: true});
315+
}, 60000);
316+
317+
test('lenient mode with aliased import also matches based on type', async () => {
318+
await withDir(async (repo) => {
319+
const tempDir = repo.path;
320+
321+
// Pattern uses the original import name, with lenient mode (default)
322+
const pat = pattern`isDate(${capture('arg')})`
323+
.configure({
324+
context: [`import { isDate } from 'node:util/types'`],
325+
// lenientTypeMatching defaults to true
326+
dependencies: {'@types/node': '^20.0.0'}
327+
});
328+
329+
let matchFound = false;
330+
let capturedArg: any = undefined;
331+
332+
spec.recipe = fromVisitor(new class extends JavaScriptVisitor<any> {
333+
override async visitMethodInvocation(methodInvocation: J.MethodInvocation, _p: any): Promise<J | undefined> {
334+
const m = await pat.match(methodInvocation);
335+
if (m) {
336+
matchFound = true;
337+
capturedArg = m.get('arg');
338+
}
339+
return methodInvocation;
340+
}
341+
});
342+
343+
await spec.rewriteRun(
344+
npm(
345+
tempDir,
346+
//language=typescript
347+
typescript(
348+
`
349+
import { isDate as checkDate } from 'node:util/types';
350+
351+
const result = checkDate(new Date());
352+
`
353+
),
354+
//language=json
355+
packageJson(
356+
`
357+
{
358+
"name": "test",
359+
"version": "1.0.0",
360+
"dependencies": {
361+
"@types/node": "^20.0.0"
362+
}
363+
}
364+
`
365+
)
366+
)
367+
);
368+
369+
// With lenient type matching, if types exist and match, aliased imports should also match
370+
expect(matchFound).toBe(true);
371+
expect(capturedArg).toBeDefined();
372+
}, {unsafeCleanup: true});
373+
}, 60000);
374+
375+
test('aliased import matching without type attribution (import-based resolution)', async () => {
376+
// Pattern uses the original import name
377+
// Testing if parser tracks import origins (module + original name) without type attribution
378+
const pat = pattern`isDate(${capture('arg')})`
379+
.configure({
380+
context: [`import { isDate } from 'node:util/types'`]
381+
// Note: NO dependencies - pattern won't have type attribution
382+
});
383+
384+
let matchFound = false;
385+
let capturedArg: any = undefined;
386+
387+
spec.recipe = fromVisitor(new class extends JavaScriptVisitor<any> {
388+
override async visitMethodInvocation(methodInvocation: J.MethodInvocation, _p: any): Promise<J | undefined> {
389+
const m = await pat.match(methodInvocation);
390+
if (m) {
391+
matchFound = true;
392+
capturedArg = m.get('arg');
393+
}
394+
return methodInvocation;
395+
}
396+
});
397+
398+
// Test code also has NO type attribution (plain typescript() without npm/dependencies)
399+
await spec.rewriteRun(
400+
//language=typescript
401+
typescript(
402+
`
403+
import { isDate as checkDate } from 'node:util/types';
404+
405+
const result = checkDate(new Date());
406+
`
407+
)
408+
);
409+
410+
// If this matches, it means the parser tracks import metadata (module + original export name)
411+
// even without full type resolution, allowing import-based alias resolution
412+
// If this fails, we currently require full type attribution for alias matching
413+
expect(matchFound).toBe(true);
414+
expect(capturedArg).toBeDefined();
415+
});
416+
417+
test.skip('pattern matches both named and namespace imports (react vs React)', async () => {
418+
await withDir(async (repo) => {
419+
const tempDir = repo.path;
420+
421+
// Pattern uses named import: forwardRef()
422+
const pat = pattern`forwardRef(${capture('fn')})`
423+
.configure({
424+
context: [`import { forwardRef } from 'react'`],
425+
dependencies: {'@types/react': '^18.0.0'}
426+
});
427+
428+
let namedImportMatched = false;
429+
let namespaceImportMatched = false;
430+
431+
spec.recipe = fromVisitor(new class extends JavaScriptVisitor<any> {
432+
override async visitMethodInvocation(methodInvocation: J.MethodInvocation, _p: any): Promise<J | undefined> {
433+
const m = await pat.match(methodInvocation);
434+
if (m) {
435+
const select = methodInvocation.select;
436+
if (!select) {
437+
namedImportMatched = true; // forwardRef(...)
438+
} else if (select.element.kind === J.Kind.Identifier) {
439+
namespaceImportMatched = true; // React.forwardRef(...)
440+
}
441+
}
442+
return methodInvocation;
443+
}
444+
});
445+
446+
await spec.rewriteRun(
447+
npm(
448+
tempDir,
449+
//language=typescript
450+
typescript(
451+
`
452+
import { forwardRef } from 'react';
453+
import * as React from 'react';
454+
455+
// Named import (module: 'react')
456+
const A = forwardRef(() => null);
457+
458+
// Namespace import (class/interface: 'React')
459+
const B = React.forwardRef(() => null);
460+
`
461+
),
462+
//language=json
463+
packageJson(
464+
`
465+
{
466+
"name": "test",
467+
"version": "1.0.0",
468+
"dependencies": {
469+
"@types/react": "^18.0.0"
470+
}
471+
}
472+
`
473+
)
474+
)
475+
);
476+
477+
// Pattern should match both forms (case-insensitive FQN: 'react' vs 'React')
478+
expect(namedImportMatched).toBe(true);
479+
expect(namespaceImportMatched).toBe(true);
480+
}, {unsafeCleanup: true});
481+
}, 60000);
258482
});

0 commit comments

Comments
 (0)
⚡