fix(type-resolution): review fixes, sizeBefore optimization, and test coverage

Address code review findings from PR #392 senior compiler review:
- Fix Java "Yes" → "No" in optional-param-arity matrix (Java has no defaults)
- Simplify Kotlin hasDefaultValue while-as-if to direct const/if check
- Update OPTIONAL_PARAM_TYPES comment to include Ruby
- Replace per-declaration Set allocation with size-based Map iteration skip
- Add 11 unit tests for multi-declarator type association and constructorTypeMap
This commit is contained in:
Gergo Magyar 2026-03-20 08:45:42 +00:00
parent c3a2815186
commit 228c993bb7
4 changed files with 207 additions and 11 deletions

View file

@ -827,13 +827,17 @@ export const buildTypeEnv = (
}
}
// Run the language-specific declaration extractor (may or may not add to scopeEnv).
const keysBefore = typeNode ? new Set(scopeEnv.keys()) : undefined;
const sizeBefore = typeNode ? scopeEnv.size : -1;
config.extractDeclaration(node, scopeEnv);
// Fallback: for multi-declarator languages (TS, C#, Java) where the type field
// is on variable_declarator children, capture via keysBefore/keysAfter diff.
if (typeNode && keysBefore) {
// is on variable_declarator children, capture newly-added keys.
// Map preserves insertion order, so new keys are always at the end —
// skip the first sizeBefore entries to find only newly-added variables.
if (sizeBefore >= 0 && scopeEnv.size > sizeBefore) {
let skip = sizeBefore;
for (const varName of scopeEnv.keys()) {
if (!keysBefore.has(varName) && !declarationTypeNodes.has(`${scope}\0${varName}`)) {
if (skip > 0) { skip--; continue; }
if (!declarationTypeNodes.has(`${scope}\0${varName}`)) {
declarationTypeNodes.set(`${scope}\0${varName}`, typeNode);
}
}
@ -850,9 +854,10 @@ export const buildTypeEnv = (
// When a declaration has BOTH a type annotation AND a constructor initializer,
// record the constructor type for receiver override at call resolution time.
// e.g., `Animal a = new Dog()` → constructorTypeMap.set('scope\0a', 'Dog')
if (keysBefore) {
if (sizeBefore >= 0 && scopeEnv.size > sizeBefore) {
let ctorSkip = sizeBefore;
for (const varName of scopeEnv.keys()) {
if (keysBefore.has(varName)) continue;
if (ctorSkip > 0) { ctorSkip--; continue; }
const declaredType = scopeEnv.get(varName);
if (!declaredType) continue;
const ctorType = extractConstructorTypeName(node)

View file

@ -649,7 +649,7 @@ export const extractMethodSignature = (node: SyntaxNode | null | undefined): Met
/** AST node types that represent parameters with default values. */
const OPTIONAL_PARAM_TYPES = new Set([
'optional_parameter', // TypeScript: (x?: number) or (x: number = 5)
'optional_parameter', // TypeScript, Ruby: (x?: number), (x: number = 5), def f(x = 5)
'default_parameter', // Python: def f(x=5)
'typed_default_parameter', // Python: def f(x: int = 5)
'optional_parameter_declaration', // C++: void f(int x = 5)
@ -667,9 +667,8 @@ export const extractMethodSignature = (node: SyntaxNode | null | undefined): Met
}
// Kotlin: default values are siblings of the parameter node, not children.
// The AST is: parameter, =, <literal> — all at function_value_parameters level.
// Walk forward from the parameter to find an immediately following '=' token.
let sib = paramNode.nextSibling;
while (sib && (sib.type === ',' || sib.type === ')')) sib = null; // stop at , or )
// Check if the immediately following sibling is '=' (default value separator).
const sib = paramNode.nextSibling;
if (sib && sib.type === '=') return true;
return false;
};

View file

@ -3852,4 +3852,196 @@ class App {
});
});
describe('multi-declarator type association (sizeBefore optimization)', () => {
it('Java: multi-declarator captures all variable names with shared type', () => {
const tree = parse(`
class App {
void run() {
User a = getA(), b = getB();
a.save();
b.save();
}
}
`, Java);
const { env } = buildTypeEnv(tree, 'java');
expect(flatGet(env, 'a')).toBe('User');
expect(flatGet(env, 'b')).toBe('User');
});
it('Java: untyped declaration before typed does not get false type association', () => {
// `x` has no type annotation → must NOT be associated with the User type
// from the later declaration. This guards the sizeBefore skip logic.
const tree = parse(`
class App {
void run() {
var x = getX();
User user = getUser();
user.save();
}
}
`, Java);
const { env } = buildTypeEnv(tree, 'java');
expect(flatGet(env, 'user')).toBe('User');
// x should NOT have a type binding (it's untyped via var)
expect(flatGet(env, 'x')).toBeUndefined();
});
it('C#: multi-declarator with shared type captures both variables', () => {
const tree = parse(`
class App {
void Run() {
User a = GetA(), b = GetB();
a.Save();
b.Save();
}
}
`, CSharp);
const { env } = buildTypeEnv(tree, 'csharp');
expect(flatGet(env, 'a')).toBe('User');
expect(flatGet(env, 'b')).toBe('User');
});
it('Java: single declarator with type still works after optimization', () => {
const tree = parse(`
class App {
void run() {
User user = getUser();
user.save();
}
}
`, Java);
const { env } = buildTypeEnv(tree, 'java');
expect(flatGet(env, 'user')).toBe('User');
});
it('Java: for-loop resolves element type from multi-declarator typed iterable', () => {
// Tests that declarationTypeNodes is correctly populated for multi-declarator
// variables, enabling for-loop element type resolution (Strategy 1).
const tree = parse(`
class App {
void run() {
List<User> users = getUsers(), admins = getAdmins();
for (User u : users) {
u.save();
}
}
}
`, Java);
const { env } = buildTypeEnv(tree, 'java');
expect(flatGet(env, 'users')).toBe('List');
expect(flatGet(env, 'admins')).toBe('List');
expect(flatGet(env, 'u')).toBe('User');
});
});
describe('constructorTypeMap (virtual dispatch detection)', () => {
it('Java: Animal a = new Dog() populates constructorTypeMap with Dog', () => {
const tree = parse(`
class Animal {}
class Dog extends Animal {}
class App {
void run() {
Animal a = new Dog();
}
}
`, Java);
const { constructorTypeMap } = buildTypeEnv(tree, 'java');
// Find the entry for variable 'a'
let ctorType: string | undefined;
for (const [key, value] of constructorTypeMap) {
if (key.endsWith('\0a')) { ctorType = value; break; }
}
expect(ctorType).toBe('Dog');
});
it('Java: same-type constructor does NOT populate constructorTypeMap', () => {
const tree = parse(`
class User {}
class App {
void run() {
User u = new User();
}
}
`, Java);
const { constructorTypeMap } = buildTypeEnv(tree, 'java');
let found = false;
for (const [key] of constructorTypeMap) {
if (key.endsWith('\0u')) { found = true; break; }
}
expect(found).toBe(false);
});
it('TypeScript: const a: Animal = new Dog() — constructorTypeMap not populated (type on variable_declarator, not lexical_declaration)', () => {
// TS virtual dispatch for this pattern works through call-processor,
// not constructorTypeMap — the type annotation is on the child
// variable_declarator, not the outer lexical_declaration.
const tree = parse(`
class Animal {}
class Dog extends Animal {}
const a: Animal = new Dog();
`, TypeScript.typescript);
const { env, constructorTypeMap } = buildTypeEnv(tree, 'typescript');
expect(flatGet(env, 'a')).toBe('Animal');
let found = false;
for (const [key] of constructorTypeMap) {
if (key.endsWith('\0a')) { found = true; break; }
}
expect(found).toBe(false);
});
it('C++: Animal* a = new Dog() populates constructorTypeMap', () => {
const tree = parse(`
class Animal {};
class Dog : public Animal {};
void run() {
Animal* a = new Dog();
}
`, CPP);
const { constructorTypeMap } = buildTypeEnv(tree, 'cpp');
let ctorType: string | undefined;
for (const [key, value] of constructorTypeMap) {
if (key.endsWith('\0a')) { ctorType = value; break; }
}
expect(ctorType).toBe('Dog');
});
it('C#: Animal a = new Dog() populates constructorTypeMap', () => {
const tree = parse(`
class Animal {}
class Dog : Animal {}
class App {
void Run() {
Animal a = new Dog();
}
}
`, CSharp);
const { constructorTypeMap } = buildTypeEnv(tree, 'csharp');
let ctorType: string | undefined;
for (const [key, value] of constructorTypeMap) {
if (key.endsWith('\0a')) { ctorType = value; break; }
}
expect(ctorType).toBe('Dog');
});
it('C#: implicit new() does NOT populate constructorTypeMap (type from declaration)', () => {
const tree = parse(`
class Dog {}
class App {
void Run() {
Dog d = new();
}
}
`, CSharp);
const { env, constructorTypeMap } = buildTypeEnv(tree, 'csharp');
// d should be bound via declared type path
expect(flatGet(env, 'd')).toBe('Dog');
// constructorTypeMap should NOT have an entry (same type, no override needed)
let found = false;
for (const [key] of constructorTypeMap) {
if (key.endsWith('\0d')) { found = true; break; }
}
expect(found).toBe(false);
});
});
});

View file

@ -388,7 +388,7 @@ So return-type-aware receiver inference already exists in a constrained downstre
| Parameter types extracted | Yes** | No | Yes | Yes | Yes | Yes | Yes | Partial†† | No | No | No | Yes | No |
| Method overload disambiguation | Yes** | No | Yes | Yes | Yes | No | No | No | No | No | No | Yes | No |
| Constructor-visible virtual dispatch | Yes | No | Yes | Yes‡‡ | Yes | No | No | No | No | No | No | Yes§§ | No |
| Optional parameter arity resolution | Yes | No | Yes | Yes | Yes | No | No | Yes | Yes | Yes | No | Yes | No |
| Optional parameter arity resolution | Yes | No | No | Yes | Yes | No | No | Yes | Yes | Yes | No | Yes | No |
\* Python class-level annotated attributes (`address: Address`) now resolve `declaredType` correctly. The `self.x` instance attribute pattern is not yet supported.