Fix17667 #21425

Lazarus535 · 2018-01-26T16:26:01Z

Some parts are pretty "fragile" still. However bugs have clearly been identified and prototype fix works. Test cases are missing.

@Mhehazy

Notes about the bug: There were in fact two bugs, as was initially suggested by @Mhehazy. The first lead to properties not being found inside optional object binding patterns. The second lead to a function not being assignable to another when it actually should have. Both bugs were fixed. Addes more testcases.

sandersn

I don't think the basic approach to add undefined recursively to the type is correct. I'll take a look tomorrow to see what will be the right approach.

sandersn · 2018-01-29T22:03:34Z

src/compiler/checker.ts

@@ -8832,7 +8833,7 @@ namespace ts {
            const targetParams = target.parameters;
            for (let i = 0; i < checkCount; i++) {
                const sourceType = i < sourceMax ? getTypeOfParameter(sourceParams[i]) : getRestTypeOfSignature(source);
-                const targetType = i < targetMax ? getTypeOfParameter(targetParams[i]) : getRestTypeOfSignature(target);
+                const targetType: Type = i < targetMax ? getTypeOfParameter(targetParams[i]) : getRestTypeOfSignature(target);


This annotation isn't required, so please remove it.

Yeah....had that in for "debugging" proposes and it slipped through. :-(

sandersn · 2018-01-29T22:58:03Z

src/compiler/checker.ts

@@ -3954,7 +3954,8 @@ namespace ts {
                    if (strictNullChecks && declaration.flags & NodeFlags.Ambient && isParameterDeclaration(declaration)) {
                        parentType = getNonNullableType(parentType);
                    }
-                    const propType = getTypeOfPropertyOfType(parentType, text);
+                    const parentTypeNEUndefined = getTypeWithFacts(parentType, TypeFacts.NEUndefined);


what happens if you just overwrite parentType as in the previous statement?

parentType = getTypeWithFacts(parentType, TypeFacts.NEUndefined);

(Later) I tried it and it improved one error baseline: downlevelLetConst16.

sandersn · 2018-01-29T23:33:11Z

src/compiler/checker.ts

+
+                if (type.flags & TypeFlags.Union || type.flags & TypeFlags.Intersection) {
+                  for (let i = 0; i < propCount; i++) {
+                    const prop = rType.properties[i] as SymbolLinks;


Please use 4-space tabs.

sandersn · 2018-01-29T23:34:53Z

src/compiler/checker.ts

+                  }
+                }
+
+                if (type.flags & TypeFlags.Union || type.flags & TypeFlags.Intersection) {


TypeFlags.UnionOrIntersection

sandersn · 2018-01-29T23:36:45Z

src/compiler/checker.ts

+                const propCount = rType.properties.length;
+
+                if (rType.flags & TypeFlags.Object) {
+                  for (let i = 0; i < propCount; i++) {


for (const prop in getPropertiesOfType(rType))

This might also collapse the object vs union/intersection code paths into one.

I will look into that.

sandersn · 2018-01-29T23:38:36Z

src/compiler/checker.ts

+                        innerType = prop.type;
+                    }
+                    prop.type = innerType;
+                    rType.properties[i] = prop as Symbol;


You need to make a new type instead of overwriting the properties of an existing type. Somebody else might expect the properties of that type to remain the same.

Same with symbol; you can't just update the type of an existing symbol.

That seems reasonable. I should wipe Haskell out of my head for good. :-D

sandersn · 2018-01-29T23:42:08Z

src/compiler/checker.ts

+
+                if (type.flags & TypeFlags.Union || type.flags & TypeFlags.Intersection) {
+                  for (let i = 0; i < propCount; i++) {
+                    const prop = rType.properties[i] as SymbolLinks;


This only works if the union has already had its properties resolved. getPropertiesOfType does the right thing.

sandersn · 2018-01-29T23:52:57Z

src/compiler/checker.ts

@@ -3954,7 +3954,8 @@ namespace ts {
                    if (strictNullChecks && declaration.flags & NodeFlags.Ambient && isParameterDeclaration(declaration)) {
                        parentType = getNonNullableType(parentType);
                    }
-                    const propType = getTypeOfPropertyOfType(parentType, text);
+                    const parentTypeNEUndefined = getTypeWithFacts(parentType, TypeFacts.NEUndefined);


It looks like undefined gets removed. whether or not there is an initialiser. I don't think this is correct.

Shouldn't there be an equivalent line for arrays? You can do things like ([first = 1, second = 2]) => first + second, right?

Here undefined should not be removed, but getTypeOfPropertyOfType should "see" through the levels of Union types (with undefined) and get the correct type of the property. Maybe the best approach here is to fix getTypeOfPropertyOfType instead of patching it here.

The problem originates in getPropertyOfUnionOrIntersectionType(), where the return type is undefined because the property is partial (only present in one of the constituents of the union).
This makes sense for some cases, but not in this (buggy) one. Probably the most "elegant" solution is to pass some sort of flag down the line to indicate if we want partials resolved or not.

sandersn · 2018-01-29T23:56:50Z

src/compiler/checker.ts

+              return type;
+            }
+        }
+
        function getTypeOfParameter(symbol: Symbol) {
            const type = getTypeOfSymbol(symbol);


I think that you need to follow getTypeOfSymbol down to where the type of binding elements is recursively obtained, and change the innermost case, instead of making a post-processing step that introduces undefined to the type after the fact.

I did exactly that in one of my fix prototypes. It broke a dozen or so of testcases. Furthermore, i don't think this approach is correct. Consider this: getTypeOfSymbol is also used "inside" a function to determine what type a argument has, as well as the signature of the function from the "outside". In this case the two are different. If there is an initializer, the argument in the function should not include undefined, because it is garantueed to never assume this value, but the signature should, because you can safely pass an undefined. We already did that actually, but just for the most "outer level" (see getTypeOfParameter), which is not correct as it has been previously. I agree that this solution (apart from the obvious improvements mentioned) is not perfect, but without changing typechecking on a more fundamental level, i don't see any other way.

typescript-bot · 2018-06-13T17:10:59Z

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

Lazarus535 added 4 commits January 26, 2018 02:57

Initial fix for # 17667

2a008c2

Some parts are pretty "fragile" still. However bugs have clearly been identified and prototype fix works. Test cases are missing.

Merge branch 'master' into fix17667

45c820f

Erased code used for debugging.

4b5193c

sandersn self-requested a review January 26, 2018 17:01

Further cleanup.

35a2047

sandersn requested changes Jan 29, 2018

View reviewed changes

typescript-bot closed this Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix17667 #21425

Fix17667 #21425

Lazarus535 commented Jan 26, 2018

sandersn left a comment

sandersn Jan 29, 2018

Lazarus535 Jan 30, 2018

sandersn Jan 29, 2018

sandersn Jan 29, 2018

sandersn Jan 29, 2018

Lazarus535 Jan 30, 2018

sandersn Jan 29, 2018

Lazarus535 Jan 30, 2018

sandersn Jan 29, 2018

Lazarus535 Jan 30, 2018

sandersn Jan 29, 2018

sandersn Jan 29, 2018

Lazarus535 Jan 30, 2018

sandersn Jan 29, 2018

sandersn Jan 29, 2018

Lazarus535 Jan 30, 2018

Lazarus535 Jan 30, 2018

sandersn Jan 29, 2018

Lazarus535 Jan 30, 2018 •

edited

Loading

typescript-bot commented Jun 13, 2018

Fix17667 #21425

Fix17667 #21425

Conversation

Lazarus535 commented Jan 26, 2018

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lazarus535 Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

typescript-bot commented Jun 13, 2018

Lazarus535 Jan 30, 2018 •

edited

Loading