Add undefined to default-initialised parameters by sandersn · Pull Request #12033 · microsoft/TypeScript

sandersn · 2016-11-03T22:39:25Z

Fixes #11900

When --strictNullChecks is on

When --strictNullChecks is on Also update a baseline that changes now.

RyanCavanaugh · 2016-11-03T23:22:42Z

tests/baselines/reference/optionalMethods.js

 declare class Bar {
    d: number;
-    e: number;
+    e: number | undefined;


This is incorrect -- because there was a non-undefined initializer, the property should not include undefined

They are actually properties, so don't get undefined added when they have an initialiser. Also update baselines

sandersn · 2016-12-08T00:12:16Z

As @blakeembrey points out in #12726, default parameters should only add undefined to the type for the signature. In the body of the function, the parameters should not add undefined.

sandersn · 2016-12-09T21:32:12Z

This is not the right fix. The behaviour is correct today -- you can pass undefined to a default-initialised parameter that's not last, and the compiler accepts it. And inside the body undefined is not part of the parameter. The correct fix is either to write ? in the signature, or not to change anything.

sandersn · 2016-12-14T00:08:11Z

Actually, this is the right fix. It just needs to improve control flow to remove the added | undefined in the body of the function. I'll add the commits for that as soon as I get the .d.ts output corrected.

Also a little code to emit them in declarations, but this doesn't work yet.

They don't narrow the parameter type, except to remove undefined, and only if the initialiser type doesn't include undefined.

mhegazy · 2016-12-30T21:47:50Z

src/compiler/declarationEmitter.ts

-                writeTypeOfDeclaration(node, node.type, getParameterDeclarationTypeVisibilityError);
+                // use the checker's type, not the declared type, for an initialized parameter (that isn't a parameter property)
+                const isInitializedParameter = node.initializer && !(getModifierFlags(node) & ModifierFlags.ParameterPropertyModifier);
+                const typeNode = isInitializedParameter ? undefined : node.type;


so we treat foo(x?: string = "") different from foo(x?:string) ?

i think with this the first will be foo(x?: string | undefined) and for the second foo(x?: string). i would say both should be the same.

That seems like a good idea. Two notes though:

We must not have very much declaration coverage because only two other tests broke.

This change makes it clearer than ever that duplicate marking of optionality is the way to go with our current semantics (which ignores the missing-undefined vs provided-undefined distinction).

in declarationEmitter.ts

sandersn · 2017-01-06T18:50:32Z

@mhegazy I think this is ready to go now if you want to take one more look.

Related to adding undefined, though not strictly the same, this change adds '?' to unused IIFE parameters in quick info.

ahejlsberg · 2017-01-27T17:32:13Z

I don't think this PR is the right fix. The type of a parameter that has an initializer should only include undefined when viewed as part of a parameter list. Within the actual function we don't want undefined in the type--as demonstrated by the PR it is a lot of work to then get rid of the undefined. We're already adding undefined to the type in getTypeOfParameter, why not just do the same when we display the function signature? Everything else already works correctly best I can tell.

sandersn · 2017-01-27T21:42:34Z

@ahejlsberg The simpler fix doesn't take care of the narrowing an explicit undefined in an initialised parameter. Compare foo2 to foo1 and foo3 in the tests. All 3 have the correct signatures, but foo2 has x: string | undefined in its body even though it's guaranteed to be string.

    function foo1(x: string = "string", b: number) {
        x.length;
    }
    
    function foo2(x: string | undefined = "string", b: number) {
        x.length; // ok, should be narrowed to string
        ~
!!! error TS2532: Object is possibly 'undefined'.
    }
    
    function foo3(x = "string", b: number) {
        x.length; // ok, should be narrowed to string
    }

However, the code is so much simpler that I don't mind missing this case. I'll commit the simplification so you can see what it looks like.

Just add undefined when displaying the type. Don't actually add it to the type.

sandersn · 2017-01-27T22:33:32Z

OK, the simplified version is up now.

ahejlsberg · 2017-01-30T17:15:50Z

Regarding this error case

    function foo2(x: string | undefined = "string", b: number) {
        x.length; // ok, should be narrowed to string
        ~
!!! error TS2532: Object is possibly 'undefined'.
    }

Similarly to how we add undefined to an explicit type annotation when a parameter has a ? modifier, we should remove undefined from the type when the parameter has an initializer with a type that doesn't include undefined. The undefined would later be added back by getTypeOfParameter as appropriate. It looks to be a fairly simple change in getTypeForVariableLikeDeclaration.

sandersn · 2017-01-31T19:00:32Z

OK, undefined is now removed when a parameter has a non-undefined-containing initializer.

ahejlsberg · 2017-01-31T22:15:34Z

src/compiler/checker.ts

+            // In strict null checking mode, a default value of a binding pattern adds undefined,
+            // which should be removed to get the type of the elements
+            const func = getContainingFunction(declaration);
+            if (strictNullChecks && func && !func.body && getFalsyFlags(parentType) & TypeFlags.Undefined) {


Not sure I understand the func && !func.body check. Also, where do you check if the binding pattern has a default value?

Removal of undefined only applies to calls from declarationEmitter. But your other suggestion below (get rid of undefined for optional parameters) removes the need for this code entirely, so I'll just delete it.

ahejlsberg · 2017-01-31T22:21:17Z

src/compiler/checker.ts


-                buildTypeDisplay(getTypeOfSymbol(p), writer, enclosingDeclaration, flags, symbolStack);
+                let type = getTypeOfSymbol(p);
+                if (strictNullChecks && parameterNode.initializer && !(getModifierFlags(parameterNode) & ModifierFlags.ParameterPropertyModifier)) {


Maybe turn this into a helper function. You have the same logic duplicated in writeTypeOfDeclaration.

ahejlsberg · 2017-01-31T22:23:36Z

src/compiler/declarationEmitter.ts

-                writeTypeOfDeclaration(node, node.type, getParameterDeclarationTypeVisibilityError);
+                // use the checker's type, not the declared type,
+                // for optional parameters and initialized ones that aren't a parameter property
+                const typeShouldAddUndefined = resolver.isOptionalParameter(node) ||


Could this be moved into writeTypeOfDeclaration. Seems like you're having to do this twice.

In declarationEmitter, writeTypeOfDeclaration decides whether to call its own emitType or the checker's writeTypeOfDeclaration. I poked around and tried a few things, and I ended up creating a new TypeFormatFlags entry called AddUndefined.

ahejlsberg · 2017-01-31T22:26:42Z

tests/baselines/reference/destructureOptionalParameter.js

    a: number;
    b: string;
-}): void;
+} | undefined): void;


Seems sort of redundant to include undefined when we already have a ? on the parameter. Same in a number of cases below.

1. Add undefined only when an initialized parameter is required (not optional). 2. Create isRequiredInitializedParameter helper function 3. Call this function only once from declarationEmitter

ahejlsberg

Approved with the noted changes.

ahejlsberg · 2017-02-13T21:08:23Z

src/compiler/checker.ts

        function getTypeForBindingElement(declaration: BindingElement): Type {
            const pattern = <BindingPattern>declaration.parent;
-            const parentType = getTypeForBindingElementParent(<VariableLikeDeclaration>pattern.parent);
+            let parentType = getTypeForBindingElementParent(<VariableLikeDeclaration>pattern.parent);


Change back to const.

ahejlsberg · 2017-02-13T21:13:00Z

src/compiler/declarationEmitter.ts

+                if (shouldUseResolverType) {
+                    format |= TypeFormatFlags.AddUndefined;
+                }
+                resolver.writeTypeOfDeclaration(declaration, enclosingDeclaration, format, writer);


Use functional style:

const format = TypeFormatFlags.UseTypeOfFunction | TypeFormatFlags.UseTypeAliasValue | (shouldUseResolverType ? TypeFormatFlags.AddUndefined : 0);

sandersn added 2 commits November 3, 2016 15:35

Add undefined to default-valued parameters

995732f

When --strictNullChecks is on

Test adding undefined to default-valued parameters

286845c

When --strictNullChecks is on Also update a baseline that changes now.

msftclas added the cla-not-required label Nov 3, 2016

sandersn changed the title ~~Add undefined to default-valued parameters~~ Add undefined to default-initialised parameters Nov 3, 2016

sandersn mentioned this pull request Nov 3, 2016

Wrong function type when mixing default initialized parameters with regular parameters #11900

Closed

RyanCavanaugh reviewed Nov 3, 2016

View reviewed changes

Do not add undefined to parameter properties

00ff0e5

They are actually properties, so don't get undefined added when they have an initialiser. Also update baselines

sandersn added 2 commits December 7, 2016 16:12

Merge branch 'master' into add-undefined-to-default-valued-parameters

bb9b681

More tests:default-valued parameter+strictNullChecks

96c14de

sandersn closed this Dec 9, 2016

sandersn added 2 commits December 13, 2016 10:40

Merge branch 'master' into add-undefined-to-default-valued-parameters

d62dd29

More test cases and notes

caad486

sandersn reopened this Dec 13, 2016

msftclas added the cla-not-required label Dec 13, 2016

sandersn added 10 commits December 14, 2016 08:48

Add parameter initialisers to control flow

bb6f3ad

Also a little code to emit them in declarations, but this doesn't work yet.

More tests of parameter initialiser type

f097eaf

Merge branch 'master' into add-undefined-to-default-valued-parameters

8b29e82

Add | undefined in .d.ts for initialised params

6046318

More tests of initialised parameters adding undefined

739c083

Remove undefined when checking patterns in function declarations

cee708d

Update tests

61c742a

Change narrowing rules for parameter initialisers

6543048

They don't narrow the parameter type, except to remove undefined, and only if the initialiser type doesn't include undefined.

Test parameter initialisation narrowing rules

3b1309d

Remove old add-undefined code in getTypeOfParameter

e0bf73f

mhegazy reviewed Dec 30, 2016

View reviewed changes

sandersn added 2 commits January 5, 2017 11:15

Add | undefined to all optional properties too

156d5a9

in declarationEmitter.ts

Merge branch 'master' into add-undefined-to-default-valued-parameters

8a9ee1a

sandersn added 2 commits January 25, 2017 16:03

Merge branch 'master' into add-undefined-to-default-valued-parameters

e96b17b

isOptionalParameter says unused IIFE arguments are optional

01a9e4f

Related to adding undefined, though not strictly the same, this change adds '?' to unused IIFE parameters in quick info.

sandersn added 2 commits January 27, 2017 14:29

Remove control flow-based undefined addition

3c243db

Just add undefined when displaying the type. Don't actually add it to the type.

Update tests and baselines

bb40819

sandersn added 2 commits January 30, 2017 14:59

Remove undefined from initialized+annotated parameter type

a235d54

Test removing undefined from initialized, annotated parameters

7cf595a

ahejlsberg reviewed Jan 31, 2017

View reviewed changes

sandersn added 2 commits February 1, 2017 09:41

Address PR comments

6f7c984

1. Add undefined only when an initialized parameter is required (not optional). 2. Create isRequiredInitializedParameter helper function 3. Call this function only once from declarationEmitter

Update baselines

6328e6c

ahejlsberg approved these changes Feb 13, 2017

View reviewed changes

Address PR comments and fix lint

c2cd4f6

sandersn merged commit 46d9f37 into master Feb 13, 2017

sandersn deleted the add-undefined-to-default-valued-parameters branch February 13, 2017 21:30

jaredru mentioned this pull request Feb 22, 2017

strictNullChecks and nullable parameters with defaults #14236

Closed

mhegazy mentioned this pull request Mar 2, 2017

Type 'undefined' is not assignable to type 'string' (reduced from a null-able function parameter) #14425

Closed

This was referenced Mar 6, 2017

TypeScript 2.2.1 generates surprising null checking error #14487

Closed

Remove undefined from the type of default initialised parameters when narrowing #14498

Merged

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

sandersn commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Dec 8, 2016

Uh oh!

sandersn commented Dec 9, 2016

Uh oh!

sandersn commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Jan 6, 2017

Uh oh!

ahejlsberg commented Jan 27, 2017

Uh oh!

sandersn commented Jan 27, 2017

Uh oh!

sandersn commented Jan 27, 2017

Uh oh!

ahejlsberg commented Jan 30, 2017

Uh oh!

sandersn commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahejlsberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sandersn commented Nov 3, 2016 •

edited

Loading