Repository navigation
Always instantiate the extends clause, even in the presence of an error - #20232
Wesley Wigham (weswigham) merged 3 commits into
Conversation
Wesley Wigham (weswigham)
changed the title
…s, instead of returning unknownType
| if (numTypeParameters) { | ||
| const numTypeArguments = length(typeArguments); | ||
| if ((isJavaScriptImplicitAny || numTypeArguments >= minTypeArgumentCount) && numTypeArguments <= numTypeParameters) { | ||
| if (isJavaScriptImplicitAny || (numTypeArguments >= minTypeArgumentCount && numTypeArguments <= numTypeParameters)) { |
There was a problem hiding this comment.
we should change the name of this parameter now to be just isJavaScript
|
Mohamed Hegazy (@mhegazy) Can I get a rereview? I've had to update the code (since apparently I did have failing tests, I was rerunning the same old file because vscode wasn't actually saving my checker.ts buffer to disk until I went to close the file... but that's another story) to 1. only affect JS (for now), and 2. handle extraneous type arguments more gracefully (ie, not crash or produce follow-on errors). |
| const typeStr = typeToString(type, /*enclosingDeclaration*/ undefined, TypeFormatFlags.WriteArrayAsGenericType); | ||
| error(node, diag, typeStr, minTypeArgumentCount, typeParameters.length); | ||
| return unknownType; | ||
| if (!isJs) { |
There was a problem hiding this comment.
so what broke with the permissive behavior in TS?
There was a problem hiding this comment.
Nothing is "broken" once all the arity-based signature removals are excised and arity verification is moved into fillMissingTypeArguments - just observable behavior changes to follow-on error scenarios (which cause a bunch of baseline churn - and in TS it is not uniformly better behavior, since we fill with {} instead of any - we could change that if there's an arity mismatch); I'll open a PR on my PR to show you, here: weswigham#2
I can merge it into this PR and iterate on it if you think it's a good idea (or do a second PR after this one). I just didn't want to add it in (it is more complicated, but also likely more complete) without mentioning it to someone first. It just amounts to better error recovery, so even if it does change a bunch of any's to more specific types in the baselines (as expected), I don't think it'd even be a breaking change, actually.
...That did take longer than I expected to fully flesh out, though - partially because I had to work around our inconsistent handling of generics on psuedoconstructors (like ArrayConstructor). Namely you can write:
interface BadBehavingCtor {
new <T>(x: T): number;
new <T, U>(x: T, y: U): string;
}and those ctor return types are in no way compatable, but we don't issue an error because we don't look at the different arities' signatures simultaneously right now. It's the reason for all the complexity around getInstantiatedConstructorsForTypeArguments.
There was a problem hiding this comment.
thanks for the clarification. i think i understand now. i am not sure i understand the change in weswigham#2, but we should talk about that one on Monday.
|
Wesley Wigham (@weswigham) let's get this in and get folks to test it, we can look at weswigham#2 in more details later on. |
|
Alright |
This prevents extra downstream errors from polluting the output in
checkJs, and also leaves the component as flexibly consumable inallowJs.