Skip to content

Ensure method calls on unions get accurate types #4309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mnsaglam wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ensure method calls on unions get accurate types #4309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
274107b
Ensure method calls on unions get accurate types
mnsaglam Apr 11, 2026
7e64ae9
Format with google-java-format
mnsaglam Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions externs/es3.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,8 +676,8 @@ ReadonlyArray.prototype.join = function(opt_separator) {};
/**
* Extracts a section of an array and returns a new array.
*
* @param {?number=} begin Zero-based index at which to begin extraction.
Copy link
Copy Markdown
Contributor

@12wrigja 12wrigja Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell removing the nullability of these params is causing lots of test breakage within Google. How critical is that to your PR / overall efforts?

We might be able to land the addition of optionality, but generally removal of nullability isn't something we can land given we don't have the capacity to fix up Google at this time.

Copy link
Copy Markdown
Contributor Author

@mnsaglam mnsaglam Apr 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. I ran all the tests available in the repo using bazelisk test //:all and I thought that would basically cover most situations.

In the current situation, the type inference bails to * when .slice() over a union. Is there nothing in the repo that takes a slice over a union type like (!Uint8Array|!Array<number>) though? I thought this would be a common utility method signature and in strictCheckTypes mode this is a warning / error.

Currently the function merging requires exact argument signature match (everything except this: type and return type should be precisely the same).

closure-compiler/src/com/google/javascript/rhino/jstype/FunctionType.java

Lines 847 to 858 in 7ee76c7

/** Try to get the sup/inf of two functions by looking at the piecewise components. */
private @Nullable FunctionType tryMergeFunctionPiecewise(FunctionType other, boolean leastSuper) {
ImmutableList<Parameter> newParamsNode = null;
if (new EqualityChecker()
.setEqMethod(EqMethod.IDENTITY)
.checkParameters(this.call, other.call)) {
newParamsNode = call.getParameterList();
} else {
// If the parameters are not equal, don't try to merge them.
// Someday, we should try to merge the individual params.
return null;
}

In reality that can be relaxed quite a bit: for instance in leastSuper = true mode, we can take the set intersection of each coordinate (for each argument length in presence of optional arguments and take union over argument length). Such as precise merge would let the correct inference happen with the current externs typings.

How about (bigint|number).toString(radix). Do you think that can be implemented?

Copy link
Copy Markdown
Contributor

@12wrigja 12wrigja Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would basically cover most situations

Maybe all "well-written" code situations yes, but these definitions are synced into google3 and are used to compile ~all JavaScript at google3, and much of what is left that is in JavaScript is old and crufty and full of type-lies and so on.

Is there nothing in the repo that takes a slice over a union type like (!Uint8Array|!Array) though?

Entirely possible, just not typed like that? It could already be typed as Array<*>

(bigint|number).toString(radix). Do you think that can be implemented?

I don't yet see any issues with that, but it would be easiest for me to test that out if you separate that into a separate PR.

* @param {?number=} end Zero-based index at which to end extraction. slice
* @param {number=} begin Zero-based index at which to begin extraction.
* @param {number=} end Zero-based index at which to end extraction. slice
* extracts up to but not including end.
* @return {!Array<T>}
* @this {IArrayLike<T>|string}
Expand Down Expand Up @@ -906,8 +906,8 @@ Array.prototype.shift = function() {};
/**
* Extracts a section of an array and returns a new array.
*
* @param {?number=} begin Zero-based index at which to begin extraction.
Copy link
Copy Markdown
Contributor

@12wrigja 12wrigja Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

* @param {?number=} end Zero-based index at which to end extraction. slice
* @param {number=} begin Zero-based index at which to begin extraction.
* @param {number=} end Zero-based index at which to end extraction. slice
* extracts up to but not including end.
* @return {!Array<T>}
* @this {IArrayLike<T>|string}
Expand Down Expand Up @@ -1188,13 +1188,13 @@ Number.prototype.toPrecision = function(opt_precision) {};
/**
* Returns a string representing the number.
* @this {Number|number}
* @param {(number|Number)=} opt_radix An optional radix.
* @param {number=} radix An optional radix.
* @return {string}
* @nosideeffects
* @see http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toString
* @override
*/
Number.prototype.toString = function(opt_radix) {};
Number.prototype.toString = function(radix) {};

// Properties.
/**
Expand Down Expand Up @@ -2178,13 +2178,13 @@ String.prototype.search = function(pattern) {};

/**
* @this {String|string}
* @param {number} begin
* @param {number=} opt_end
* @param {number=} begin
* @param {number=} end
* @return {string}
* @nosideeffects
* @see http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/slice
*/
String.prototype.slice = function(begin, opt_end) {};
String.prototype.slice = function(begin, end) {};

/**
* @this {String|string}
Expand Down
6 changes: 3 additions & 3 deletions externs/es6.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,12 +389,12 @@ function ArrayBuffer(length) {}
ArrayBuffer.prototype.byteLength;

/**
* @param {number} begin
* @param {number=} opt_end
* @param {number=} begin
* @param {number=} end
* @return {!ArrayBuffer}
* @nosideeffects
*/
ArrayBuffer.prototype.slice = function(begin, opt_end) {};
ArrayBuffer.prototype.slice = function(begin, end) {};

/**
* @param {*} arg
Expand Down
168 changes: 156 additions & 12 deletions src/com/google/javascript/jscomp/testing/TestExternsBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,87 @@ function BigInt(arg) {}
BigInt.prototype.valueOf = function() {};
""";

private static final String NUMBER_EXTERNS =
"""
/**
* @constructor
* @param {*=} arg
* @return {number}
*/
function Number(arg) {}

/**
* @this {Number|number}
* @param {number=} radix
* @return {string}
*/
Number.prototype.toString = function(radix) {};

/**
* @return {number}
*/
Number.prototype.valueOf = function() {};
""";

private static final String UINT8ARRAY_EXTERNS =
"""
/**
* @constructor
* @implements {IArrayLike<number>}
* @param {*=} arg
* @param {number=} opt_byteOffset
* @param {number=} opt_length
*/
function Uint8Array(arg, opt_byteOffset, opt_length) {}

/** @type {number} */
Uint8Array.prototype.length;

/**
* @param {number=} begin
* @param {number=} end
* @return {!Uint8Array}
* @nosideeffects
*/
Uint8Array.prototype.slice = function(begin, end) {};

/**
* @param {number} searchElement
* @param {number=} opt_fromIndex
* @return {number}
* @nosideeffects
*/
Uint8Array.prototype.indexOf = function(searchElement, opt_fromIndex) {};

/**
* @param {number} searchElement
* @param {number=} opt_fromIndex
* @return {boolean}
* @nosideeffects
*/
Uint8Array.prototype.includes = function(searchElement, opt_fromIndex) {};
""";

private static final String ARRAYBUFFER_EXTERNS =
"""
/**
* @constructor
* @param {number} length
*/
function ArrayBuffer(length) {}

/** @type {number} */
ArrayBuffer.prototype.byteLength;

/**
* @param {number=} begin
* @param {number=} end
* @return {!ArrayBuffer}
* @nosideeffects
*/
ArrayBuffer.prototype.slice = function(begin, end) {};
""";

private static final String ITERABLE_EXTERNS =
"""
// Symbol is needed for Symbol.iterator
Expand Down Expand Up @@ -274,17 +355,22 @@ function String(arg) {}
String.prototype[Symbol.iterator] = function() {};
/** @type {number} */
String.prototype.length;
/** @param {number} sliceArg */
String.prototype.slice = function(sliceArg) {};
/**
* @this {string|!String}
* @this {!String|string}
* @param {number=} begin
* @param {number=} end
* @return {string}
*/
String.prototype.slice = function(begin, end) {};
/**
* @this {!String|string}
* @param {*=} opt_separator
* @param {number=} opt_limit
* @return {!Array<string>}
*/
String.prototype.split = function(opt_separator, opt_limit) {};
/**
* @this {string|!String}
* @this {!String|string}
* @param {string} search_string
* @param {number=} opt_position
* @return {boolean}
Expand All @@ -305,6 +391,12 @@ function String(arg) {}
* @return {string}
*/
String.prototype.charAt = function(index) {};
/**
* @this {!String|string}
* @param {...*} var_args
* @return {string}
*/
String.prototype.concat = function(var_args) {};
/**
* @this {!String|string}
* @param {*} regexp
Expand All @@ -316,22 +408,27 @@ function String(arg) {}
* @return {string}
*/
String.prototype.toLowerCase = function() {};

/**
* @param {number} count
* @this {!String|string}
* @return {string}
* @nosideeffects
*/
String.prototype.repeat = function(count) {};

/**
* @param {string} searchString
* @param {number=} position
* @return {boolean}
* @nosideeffects
*/
String.prototype.includes = function(searchString, position) {};
/**
* @this {!String|string}
* @param {string|null} searchValue
* @param {(number|null)=} fromIndex
* @return {number}
*/
String.prototype.indexOf = function(searchValue, fromIndex) {};
""";
private static final String FUNCTION_EXTERNS =
"""
Expand Down Expand Up @@ -521,21 +618,27 @@ function ReadonlyArray(var_args) {}
*/
ReadonlyArray.prototype.concat;
/**
* @param {?number=} begin Zero-based index at which to begin extraction.
* @param {?number=} end Zero-based index at which to end extraction. slice
* @param {number=} begin Zero-based index at which to begin extraction.
* @param {number=} end Zero-based index at which to end extraction. slice
* extracts up to but not including end.
* @return {!Array<T>}
* @this {!IArrayLike<T>|string}
* @template T
* @nosideeffects
*/
ReadonlyArray.prototype.slice;

/**
* @param {T} elem
* @param {number=} fromIndex
* @return {number}
* @this {!IArrayLike<T>|string}
* @template T
*/
ReadonlyArray.prototype.indexOf = function(elem, fromIndex) {};
/**
* @return {!IteratorIterable<T>}
*/
ReadonlyArray.prototype.values;

/**
* @param {T} searchElement
* @param {number=} fromIndex
Expand Down Expand Up @@ -614,8 +717,8 @@ function Array(var_args) {}
Array.prototype.concat = function(var_args) {};
/**
* @override
* @param {?number=} begin Zero-based index at which to begin extraction.
* @param {?number=} end Zero-based index at which to end extraction. slice
* @param {number=} begin Zero-based index at which to begin extraction.
* @param {number=} end Zero-based index at which to end extraction. slice
* extracts up to but not including end.
* @return {!Array<T>}
* @this {!IArrayLike<T>|string}
Expand All @@ -640,6 +743,16 @@ function Array(var_args) {}
* @nosideeffects
*/
Array.prototype.includes = function(searchElement, fromIndex) {};

/**
* @param {T} elem
* @param {number=} fromIndex
* @return {number}
* @this {!IArrayLike<T>|string}
* @template T
* @override
*/
Array.prototype.indexOf = function(elem, fromIndex) {};
""";
private static final String MAP_EXTERNS =
"""
Expand Down Expand Up @@ -1060,6 +1173,9 @@ function Polymer_LegacyElementMixin(){}
""";

private boolean includeBigIntExterns = false;
private boolean includeNumberExterns = false;
private boolean includeUint8ArrayExterns = false;
private boolean includeArrayBufferExterns = false;
private boolean includeIterableExterns = false;
private boolean includeStringExterns = false;
private boolean includeFunctionExterns = false;
Expand Down Expand Up @@ -1091,6 +1207,25 @@ public TestExternsBuilder addBigInt() {
return this;
}

@CanIgnoreReturnValue
public TestExternsBuilder addNumber() {
includeNumberExterns = true;
return this;
}

@CanIgnoreReturnValue
public TestExternsBuilder addUint8Array() {
includeUint8ArrayExterns = true;
addArray(); // Uint8Array implements IArrayLike<number>
return this;
}

@CanIgnoreReturnValue
public TestExternsBuilder addArrayBuffer() {
includeArrayBufferExterns = true;
return this;
}

@CanIgnoreReturnValue
public TestExternsBuilder addIterable() {
includeIterableExterns = true;
Expand Down Expand Up @@ -1273,6 +1408,9 @@ public String build() {
if (includeBigIntExterns) {
externSections.add(BIGINT_EXTERNS);
}
if (includeNumberExterns) {
externSections.add(NUMBER_EXTERNS);
}
if (includeIterableExterns) {
externSections.add(ITERABLE_EXTERNS);
}
Expand All @@ -1288,6 +1426,12 @@ public String build() {
if (includeArrayExterns) {
externSections.add(ARRAY_EXTERNS);
}
if (includeUint8ArrayExterns) {
externSections.add(UINT8ARRAY_EXTERNS);
}
if (includeArrayBufferExterns) {
externSections.add(ARRAYBUFFER_EXTERNS);
}
if (includeMapExterns) {
externSections.add(MAP_EXTERNS);
}
Expand Down
4 changes: 2 additions & 2 deletions test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,13 +764,13 @@ public void testReplaceWithCharAt() {
foldSame(
"""
/** @constructor */ function A() {};
A.prototype.substring = function(begin$jscomp$1, end$jscomp$1) {};
A.prototype.substring = function(begin$jscomp$2, end$jscomp$2) {};
function f(/** !A */ a) { a.substring(0, 1); }
""");
foldSame(
"""
/** @constructor */ function A() {};
A.prototype.slice = function(begin$jscomp$1, end$jscomp$1) {};
A.prototype.slice = function(begin$jscomp$2, end$jscomp$2) {};
function f(/** !A */ a) { a.slice(0, 1); }
""");

Expand Down
11 changes: 7 additions & 4 deletions test/com/google/javascript/jscomp/SymbolTableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,12 +1626,15 @@ public void testSymbolForScopeOfNatives() {
SymbolTable table = createSymbolTableWithDefaultExterns("");

// From the externs.
Symbol sliceArg = getLocalVar(table, "sliceArg");
assertThat(sliceArg).isNotNull();
Symbol slice = getGlobalVar(table, "String.prototype.slice");
assertThat(slice).isNotNull();

Symbol scope = table.getSymbolForScope(table.getScope(sliceArg));
Symbol begin = table.getParameterInFunction(slice, "begin");
assertThat(begin).isNotNull();

Symbol scope = table.getSymbolForScope(table.getScope(begin));
assertThat(scope).isNotNull();
assertThat(getGlobalVar(table, "String.prototype.slice")).isEqualTo(scope);
assertThat(slice).isEqualTo(scope);

Symbol proto = getGlobalVar(table, "String.prototype");
assertThat(proto.getDeclaration().getNode().getSourceFileName()).isEqualTo("externs1");
Expand Down
2 changes: 1 addition & 1 deletion test/com/google/javascript/jscomp/TypeCheckCovarianceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1462,7 +1462,7 @@ function MyArray() {}
found : MyArray<number,number>
required: ReadonlyArray<*>
missing : []
mismatch: [includes,takes]
mismatch: [includes,indexOf,takes]
""")
.addDiagnostic(
// this diagnostic is correct - ReadonlyArray<number>.returns() returns a number, while
Expand Down
Loading