Look for usable type nodes in associated expressions for declaration emit #57772
Conversation
src/compiler/checker.ts
Outdated
| @@ -8525,7 +8589,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const sym = resolveEntityName(leftmost, SymbolFlags.All, /*ignoreErrors*/ true, /*dontResolveAlias*/ true); | |||
| if (sym) { | |||
| if (isSymbolAccessible(sym, context.enclosingDeclaration, SymbolFlags.All, /*shouldComputeAliasesToMakeVisible*/ false).accessibility !== SymbolAccessibility.Accessible) { | |||
| introducesError = true; | |||
| if (!isDeclarationName(node)) { | |||
There was a problem hiding this comment.
This single line is the bugfix I mention in the OP - this is sufficient for most cases, but not complete. A (x: whatever) => typeof x node we try to reuse will no longer be rejected on the first x in the parameter list, so we'll use the parameters and signature, but the typeof x will still be rejected, since x isn't actually "in scope" in the scope we're considering. This could be fixed by invoking the same "fake scope" logic we already use in signatureToSignatureDeclarationHelper for wholly synthetic signatures to stack up the signature scopes we're trying to preserve as we descend, but since that doesn't seem to be relevant for any tests here, I've left that for future work for now, since it's a slight refactor to extract the "fake scope" helper functions into the outer scope.
src/compiler/checker.ts
Outdated
| @@ -8664,6 +8734,16 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| node.isTypeOf, | |||
| ); | |||
| } | |||
| if (isParameter(node)) { | |||
| if (!node.type && !node.initializer) { | |||
| return factory.updateParameterDeclaration(node, /*modifiers*/ undefined, node.dotDotDotToken, visitEachChild(node.name, visitExistingNodeTreeSymbols, /*context*/ undefined), node.questionToken, factory.createKeywordTypeNode(SyntaxKind.AnyKeyword), /*initializer*/ undefined); | |||
There was a problem hiding this comment.
This was kinda of just a drive-by fix - when looking at the whitespace changes, I noticed that some retained nodes in types baselines were missing : any because the input didn't have them. This probably never comes up in real code, since declaration emit doesn't trigger if the input code has errors, and basically everyone (our tests included) has noImplicitAny on. Still, .types baselines are made (and error messages!) - so this should (and does) help those cases look more complete (not that type portability across compiler settings matters for them).
| @@ -8673,7 +8753,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
| } | |||
|
|
|||
| if (file && isTupleTypeNode(node) && (getLineAndCharacterOfPosition(file, node.pos).line === getLineAndCharacterOfPosition(file, node.end).line)) { | |||
| if (file && isTupleTypeNode(node) && !nodeIsSynthesized(node) && (getLineAndCharacterOfPosition(file, node.pos).line === getLineAndCharacterOfPosition(file, node.end).line)) { | |||
There was a problem hiding this comment.
I toyed with expanding this make-tuples-single-line-if-the-input-was-single-line to all object types, which works, but affects a lot of baselines. And it just made me notice the other cases where it wasn't retained even more, because the declaration emitter, when it keeps around an existing node, also doesn't set SingleLine. In the end, I decided messing with the whitespace was a bit too far out of scope, so left that for another day. But what I kept was this nodeIsSynthesized check - getLineAndCharacterOfPosition asserts if it gets a -1 for a position. When I was doing it for more nodes, I found this can crash, because the nodes may already have been transformed. I could put together a test for said crash, now that I know it's there, but I'm not sure what passes in a node with synthetic positions into this function - maybe something via a quickfix? The fix is just this, at least.
src/compiler/checker.ts
Outdated
| @@ -9956,6 +10036,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| // whose input is not type annotated (if the input symbol has an annotation we can reuse, we should prefer it) | |||
| const ctxSrc = getSourceFileOfNode(context.enclosingDeclaration); | |||
| return getObjectFlags(typeToSerialize) & (ObjectFlags.Anonymous | ObjectFlags.Mapped) && | |||
| !some(typeToSerialize.symbol?.declarations, isTypeNode) && // If the type comes straight from a type node, we shouldn't try to break it up | |||
There was a problem hiding this comment.
The JS declaration emit example is, apparently, the first time we've printed multiple variables of the same type where that type is defined in the current file in js declaration emit tests. It revealed that we tried to print them all as a namespace, but all the namespaces after the first would have no members, since they already got printed. That's silly. The namespace output mode for JS declaration emit is really supposed to be for things merged together across a lot of definitions - we can be pretty sure that if the type comes from a type node (as opposed to, say a binary expression or property access expression), we shouldn't convert it to a namespace (since, like, just writing a type node should be able to perfectly capture a type from a type node).
src/compiler/checker.ts
Outdated
| : !!(declaration as HasInitializer).initializer | ||
| ? (declaration as HasInitializer & typeof declaration).initializer | ||
| : isParameter(declaration) && isSetAccessor(declaration.parent) | ||
| ? getSingleReturnExpression(getAllAccessorDeclarations(getSymbolOfDeclaration(declaration.parent)?.declarations, declaration.parent).getAccessor) |
There was a problem hiding this comment.
The accessor case in the original PR not working made me sad, so I added this line and made it work. We're smart and can look at multiple places for the potential annotation, after all.
src/compiler/checker.ts
Outdated
| @@ -8460,17 +8525,15 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| * Unlike `typeToTypeNodeHelper`, this handles setting up the `AllowUniqueESSymbolType` flag | |||
| * so a `unique symbol` is returned when appropriate for the input symbol, rather than `typeof sym` | |||
| */ | |||
| function serializeTypeForDeclaration(context: NodeBuilderContext, type: Type, symbol: Symbol, enclosingDeclaration: Node | undefined, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean) { | |||
| function serializeTypeForDeclaration(context: NodeBuilderContext, type: Type, symbol: Symbol, enclosingDeclaration: Node | undefined, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean, addUndefined?: boolean) { | |||
There was a problem hiding this comment.
Honestly, we can probably delete the addUndefined parameter with a bit more work - this function can rederive the same thing with the arguments it's given. Given that, since these functions do attempt top-level node reuse now, it'd also be nice to just ditch requiresAddingImplicitUndefined from the EmitResolver entirely, and let the resolver's internals figure it out when you call serializeTypeForDeclaration via createTypeOfDeclaration. I actually tried this for a second, and it seems like the only major blocker is that createReturnTypeOfSignatureDeclaration actually isn't what we want - we need it to return type predicate nodes, too! You can't notice the issue today, since all predicates have to be annotated, so they get preserved in the declaration emitter, but #57465 will change this! It's very likely the structure of that code will change a bit in response to that PR, and then maybe removing addUndefined and requiresAddingImplicitUndefined will feel more easily done.
There was a problem hiding this comment.
but #57465 will change this
Would it be good to proactively add a test case for that?
There was a problem hiding this comment.
It adds its' own tests for it and since you can't cast into a type predicate like null! as x is number, I don't think this PR has much to add on top of that. But it should still make the refactor I mentioned above easier to do - though I don't really want to do it here, since it'll probably require fixing some more discrepancies between the declaration emitter's existing type node reuse logic and the node builder's inner type node reuse logic, as while the later is generalized to apply at the top level (using serializeTypeForDeclaration on the top-level type node if present), I've not yet done a comparison to see if the behaviors are identical (though it's already in use for JS declarations, ofc).
|
@typescript-bot pack this |
|
Hey @DanielRosenwasser, 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 |
|
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
|
@DanielRosenwasser Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. DetailsServer exited prematurely with code unknown and signal SIGABRTAffected reposcalcom/cal.comRaw error text:RepoResults7/calcom.cal.com.rawError.txt in the artifact folder
Last few requests{"seq":968,"type":"request","command":"getOutliningSpans","arguments":{"file":"@PROJECT_ROOT@/packages/app-store/utils.ts"}}
{"seq":969,"type":"request","command":"navtree","arguments":{"file":"@PROJECT_ROOT@/packages/app-store/utils.ts"}}
{"seq":970,"type":"request","command":"navbar","arguments":{"file":"@PROJECT_ROOT@/packages/app-store/utils.ts"}}
{"seq":971,"type":"request","command":"navto","arguments":{"searchValue":"a","maxResultCount":256}}
Repro steps
|
src/compiler/checker.ts
Outdated
| @@ -8460,17 +8525,15 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| * Unlike `typeToTypeNodeHelper`, this handles setting up the `AllowUniqueESSymbolType` flag | |||
| * so a `unique symbol` is returned when appropriate for the input symbol, rather than `typeof sym` | |||
| */ | |||
| function serializeTypeForDeclaration(context: NodeBuilderContext, type: Type, symbol: Symbol, enclosingDeclaration: Node | undefined, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean) { | |||
| function serializeTypeForDeclaration(context: NodeBuilderContext, type: Type, symbol: Symbol, enclosingDeclaration: Node | undefined, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean, addUndefined?: boolean) { | |||
There was a problem hiding this comment.
but #57465 will change this
Would it be good to proactively add a test case for that?
|
Other than my comments (and the current merge conflict) I think this is good. Any other thoughts @RyanCavanaugh @jakebailey ? |
|
I think I still feel uneasy about the behavior of parameter properties. Take export class Foo {
constructor(public bar?: string) {
}
}Our JS emit is: var Foo = /** @class */ (function () {
function Foo(bar) {
this.bar = bar;
}
return Foo;
}());
export declare class Foo {
bar?: string | undefined;
constructor(bar?: string | undefined);
}But after this PR, it's: export declare class Foo {
bar?: string;
constructor(bar?: string);
}Consumers may read this as "the class will either have I feel like maybe I'm not understanding #57772 (comment) or something, because this behavior really does look like a regression, and one of the ones that I pointed out in #53463. |
|
@jakebailey I've brought back the old emit for optional parameter properties - we can look into changing it later. It feels like they don't really play well with |
Thanks; I do wonder if we could simply preserve parameter properties in declaration emit, eliminating the need to rewrite the properties and making other emitters easier to write (just leave it as-is). But, not in this PR. |
|
(I am giving this PR one last pass now.) |
| /** @param {number} req */ | ||
| methodWithRequiredDefault(p = /** @type {P} */(something), req) {} | ||
|
|
||
| constructor(ctorField = /** @type {P} */(something)) {} |
There was a problem hiding this comment.
This looks like a typo; should be public ctorField =..., right?
There was a problem hiding this comment.
This is javascript, so no - since parameter properties are a syntax error in JS files.
There was a problem hiding this comment.
(And there's no jsdoc equivalent)
There was a problem hiding this comment.
Oh, whoops, didn't notice that.
Unfortunately, we issue |
Per our design meeting notes (#57756), to expedite things a bit, this is a version of #57589 that (re)uses the machinery we already have for node reuse within the
NodeBuilderin the checker.Honestly, a worthwhile endeavor, since the test cases from the other PR let me find a bug in the existing node reuse code where we weren't reusing any nodes with a parameter or property in them (oops). The quick fix for that is responsible for most of the test whitespace changes in this PR (reused nodes don't get
EmitFlags.SingleLineset, new nodes always do - and if that's a good thing or not is another discussion) - but I could split that short change out, if that was important for comparison (but it'd make two of the examples intests\cases\compiler\declarationEmitCastReusesTypeNode5.tsnot be copied - that's what made me notice the issue).I've also normalized the new tests a bit, so all the similar ones test all the same variations, in addition to a couple extra variations. Notable differences from the original PR include this working for JS declaration emit, for
setaccessors, and for annotated required initialized parameters. I also took the opportunity to normalize all the callers ofserializeExistingTypeNode(since I was adding another one or two) to perform the same sanity checking on the type (so all callers ensure the type node we're preserving actually resolves to the type we're looking at, and that if we're looking at a type reference, we don't try and reuse a type reference with less than the required number of type arguments (an error that is kind-of allowed in JS, but definitely results in an error if you copy it unmodified into TS)).Fixes #57587