[ID-Prep] PR3 - Preserve parameter types for optional parameters /fields with undefined in type and for required params with default value #57484
Conversation
typescript-bot
added
the
For Uncommitted Bug
| if (!getParseTreeNode(node)) { | ||
| return type ? visitNode(type, visitDeclarationSubtree, isTypeNode) : factory.createKeywordTypeNode(SyntaxKind.AnyKeyword); | ||
| } | ||
| if (node.kind === SyntaxKind.SetAccessor) { |
There was a problem hiding this comment.
ensureType is never called with a SetAccessorDeclaration. The set accessor type does not have a return type and the parameter type is inferred independently. Removing this had 0 impact on tests.
| if (isPropertySignature(node) || !node.initializer) return cleanup(resolver.createTypeOfDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker, shouldUseResolverType)); | ||
| return cleanup(resolver.createTypeOfDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker, shouldUseResolverType) || resolver.createTypeOfExpression(node.initializer, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker)); |
There was a problem hiding this comment.
The logic here is overly complicated. Both calls to createTypeOfDeclaration boil down to the same arguments. Also I have not found a test that benefits from the use of createTypeOfExpression on the initializer and I can't think of a case when this would help. (There might be some error case, but it's not tested for and returning any for that seems just as appropriate.
| } | ||
| return cleanup(resolver.createReturnTypeOfSignatureDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker)); |
There was a problem hiding this comment.
Removed the cleanup closure. Since the logic is simpler now it's easier to just assign a variable and do the same cleanup after the switch.
| isRequiredInitializedParameter(node: ParameterDeclaration): boolean; | ||
| isOptionalUninitializedParameterProperty(node: ParameterDeclaration): boolean; | ||
| requiresAddingImplicitUndefined(node: ParameterDeclaration): boolean; |
There was a problem hiding this comment.
isRequiredInitializedParameter and isOptionalUninitializedParameterProperty were used only once in the declaration transform in (resolver.isRequiredInitializedParameter(node) || resolver.isOptionalUninitializedParameterProperty(node)). I think it's worth simplifying the API and only expose a function taht tells us is the parameter requires adding an implcit undefined to it's type or not.
requiresAddingImplicitUndefined also takes into account if the type already contains undefined and thus avoids re-printing types unless it's absolutely necessary. (Fixing #57483)
dragomirtitian
force-pushed
the
verbatim-declarations-optional-params
branch
from
bf1ee1e
to
85f60d9
Compare
typescript-bot
added
For Backlog Bug
|
@typescript-bot test top200 @typescript-bot perf test this faster |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at a05d427. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at a05d427. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the faster perf test suite on this PR at a05d427. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at a05d427. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
To be clear, this doesn't actually fix #57483, though, right? As we don't have the follow-up to continue node reuse? |
It reuses type nodes if we know they are correct (if they already contain |
Upon further reflection, adding I think this PR gets us as close to what an external tool could reasonably emit. The cases where the type is resolved in the new tests will be errors under |
dragomirtitian
changed the title
…ed in type and for required params with default value (microsoft#57484)
Fixes #57483