add undefined to declaration of constructor parameter property with…
#50494
Conversation
There was a problem hiding this comment.
This feels like it is working against existing machinery for tracking strict null checks and emitting type declarations. @weswigham can you take a look too?
src/compiler/checker.ts
Outdated
| @@ -43537,7 +43536,7 @@ namespace ts { | |||
| flags |= NodeBuilderFlags.AllowUniqueESSymbolType; | |||
| } | |||
| if (addUndefined) { | |||
| type = getOptionalType(type); | |||
| type = getOptionalType(type, /*isProperty*/ undefined, /*ignoreStrictNullChecks*/ true); | |||
There was a problem hiding this comment.
I think a better way to fix this would be to temporarily set strictNullChecks = true and then run getOptionalType, then reset it afterward. @weswigham does that make sense to you? Or can you suggest a different way entirely?
There was a problem hiding this comment.
Any and every initialized parameter property needs a ? if it's a trailing parameter. This is something that the transformer needs to check, beyond the resolver.isOptionalParameter check it already does. Once that's handled, rather than doing anything strict-null-check-dependent, I'd keep it syntactic, and likely in the declaration emitter. If the parameter doesn't get a ? (because it has a required parameter after it), then we add | undefined to the type, regardless of mode, provided the existing serialized type doesn't contain it. (We don't need/want to do this extra explicit undefined addition for stuff with the ? so we can better strict optional property check them, I think.)
There was a problem hiding this comment.
@weswigham Very insightful! Should just think outside the box. Now the implementation is simplified and several bugs are fixed.
src/compiler/checker.ts
Outdated
| !isOptionalParameter(parameter) && | ||
| !isJSDocParameterTag(parameter) && | ||
| !!parameter.initializer && | ||
| !hasSyntacticModifier(parameter, ModifierFlags.ParameterPropertyModifier); | ||
| (!checkSyntacticModifier || !hasSyntacticModifier(parameter, ModifierFlags.ParameterPropertyModifier)); |
There was a problem hiding this comment.
I read this as: when checkSyntacticModifier=false, we are supposed to treat all parameters with initialisers as required, even if they are parameter properties. (normally parameter properties with initialisers are optional.)
Why? I kind of understand the original rule: parameter properties with initialisers don't need an argument, although I don't understand why they're special. But I don't understand why they now need an exemption. I guess it's so that the declaration emitter can distinguish between the emitted parameter (needs to add | undefined) and the property declaration (needs no undefined).
src/compiler/checker.ts
Outdated
| @@ -43537,7 +43536,7 @@ namespace ts { | |||
| flags |= NodeBuilderFlags.AllowUniqueESSymbolType; | |||
| } | |||
| if (addUndefined) { | |||
| type = getOptionalType(type); | |||
| type = getOptionalType(type, /*isProperty*/ undefined, /*ignoreStrictNullChecks*/ true); | |||
There was a problem hiding this comment.
Any and every initialized parameter property needs a ? if it's a trailing parameter. This is something that the transformer needs to check, beyond the resolver.isOptionalParameter check it already does. Once that's handled, rather than doing anything strict-null-check-dependent, I'd keep it syntactic, and likely in the declaration emitter. If the parameter doesn't get a ? (because it has a required parameter after it), then we add | undefined to the type, regardless of mode, provided the existing serialized type doesn't contain it. (We don't need/want to do this extra explicit undefined addition for stuff with the ? so we can better strict optional property check them, I think.)
Zzzen
force-pushed
the
decl-emit-for-parameter-property
branch
from
fa1542c
to
40fdbe6
Compare
There was a problem hiding this comment.
I think this approach looks pretty workable, though I think maybe we also need to consider exactOptionalPropertyTypes behavior when generating the type nodes, too. Or maybe we just need to reuse input nodes more. Most of these baselines look good, it's just the change to the non-strict parameter property declaration emit that worries me.
| @@ -248,7 +248,7 @@ export declare class ConstructorWithPrivateParameterProperty { | |||
| constructor(x: string); | |||
| } | |||
| export declare class ConstructorWithOptionalParameterProperty { | |||
| x?: string; | |||
| x?: string | undefined; | |||
There was a problem hiding this comment.
Hm, I think we'd still rather this not add undefined, since the original declaration didn't have undefined, for the sake of exactOptionalPropertyTypes. Maybe we need to consider that flag's logic during serialization as well?
There was a problem hiding this comment.
To my surprise, the implementation was simpler than I expected.
… default
Fixes #48547