Ensure method calls on unions get accurate types

[mnsaglam]

• Ensure that Array, ArrayBuffer, ReadonlyArray, String, TypedArray have consistent slice() signatures so method calls over the union gets typed correctly.

• Ensure that BigInt.prototype.toString and Number.prototype.toString are typed consistently so (bigint|number).toString() gets inferred correctly.

• Add tests for indexOf, includes, slice, toString so that the signatures of these cognate methods do not drift by accident.

Note: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/slice claims that String.prototype.slice() first argument is required at the beginning of the doc, but later admits "If indexStart is omitted, undefined, or cannot be converted to a number, it's treated as 0."

Making both params of String.prototype.slice optional is consistent with the runtime and common TypeScript typings.

[12wrigja]

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.

[mnsaglam]

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?

[12wrigja]

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.

[12wrigja]

same here.