-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Added typeToTypeNode with truncation #59332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
f6e323c
9092688
44568f7
c1b1d40
52c2ff1
05431a6
5810eab
5891f32
90dade9
3b44569
2cbb9f3
ac4ba37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import { | |
addRange, | ||
addRelatedInfo, | ||
addSyntheticLeadingComment, | ||
addSyntheticTrailingComment, | ||
AliasDeclarationNode, | ||
AllAccessorDeclarations, | ||
AmbientModuleDeclaration, | ||
|
@@ -7054,6 +7055,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
|
||
function createTypeNodesFromResolvedType(resolvedType: ResolvedType): TypeElement[] | undefined { | ||
if (checkTruncationLength(context)) { | ||
if (context.flags & NodeBuilderFlags.NoTruncation) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a test that shows what this looks like? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that you mentioned, there's no test for this scenario. I removed this check, and nothing regressed. I'm not sure how to test it but if it's utterly necessary I can address that another PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://app.codecov.io/gh/microsoft/TypeScript/pull/59332?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=microsoft implies that this code is covered somewhere in a test. Maybe add an assert and reverse engineer a specific test from that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After deeper analysis, the file Check line 18, for the last property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was working on it. Added. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to confirm, you're saying that in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I mean, before this change files in After this change, See the test |
||
return []; | ||
} | ||
return [factory.createPropertySignature(/*modifiers*/ undefined, "...", /*questionToken*/ undefined, /*type*/ undefined)]; | ||
} | ||
const typeElements: TypeElement[] = []; | ||
|
@@ -7085,7 +7089,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
} | ||
} | ||
if (checkTruncationLength(context) && (i + 2 < properties.length - 1)) { | ||
typeElements.push(factory.createPropertySignature(/*modifiers*/ undefined, `... ${properties.length - i} more ...`, /*questionToken*/ undefined, /*type*/ undefined)); | ||
if (context.flags & NodeBuilderFlags.NoTruncation) { | ||
const typeElement = typeElements.pop()!; | ||
typeElements.push(addSyntheticTrailingComment(typeElement, SyntaxKind.MultiLineCommentTrivia, `... ${properties.length - i} more ...`)); | ||
} | ||
else { | ||
typeElements.push(factory.createPropertySignature(/*modifiers*/ undefined, `... ${properties.length - i} more ...`, /*questionToken*/ undefined, /*type*/ undefined)); | ||
} | ||
addPropertyToElementList(properties[properties.length - 1], context, typeElements); | ||
break; | ||
} | ||
|
@@ -7100,7 +7110,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
if (!(context.flags & NodeBuilderFlags.NoTruncation)) { | ||
return factory.createTypeReferenceNode(factory.createIdentifier("..."), /*typeArguments*/ undefined); | ||
} | ||
return factory.createKeywordTypeNode(SyntaxKind.AnyKeyword); | ||
return addSyntheticLeadingComment(factory.createKeywordTypeNode(SyntaxKind.AnyKeyword), SyntaxKind.MultiLineCommentTrivia, "elided"); | ||
} | ||
|
||
function shouldUsePlaceholderForProperty(propertySymbol: Symbol, context: NodeBuilderContext) { | ||
|
@@ -7259,7 +7269,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
function mapToTypeNodes(types: readonly Type[] | undefined, context: NodeBuilderContext, isBareList?: boolean): TypeNode[] | undefined { | ||
if (some(types)) { | ||
if (checkTruncationLength(context)) { | ||
if (!isBareList) { | ||
if (context.flags & NodeBuilderFlags.NoTruncation) { | ||
return [addSyntheticLeadingComment(factory.createKeywordTypeNode(SyntaxKind.AnyKeyword), SyntaxKind.MultiLineCommentTrivia, "elided")]; | ||
} | ||
else if (!isBareList) { | ||
return [factory.createTypeReferenceNode("...", /*typeArguments*/ undefined)]; | ||
} | ||
else if (types.length > 2) { | ||
|
@@ -7278,6 +7291,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
for (const type of types) { | ||
i++; | ||
if (checkTruncationLength(context) && (i + 2 < types.length - 1)) { | ||
if (context.flags & NodeBuilderFlags.NoTruncation) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I confused or is this truncating things more when the flag is enabled? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not more per se, but it is replacing the previous values with an We kinda have 3 states: when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we decide to remove all of the types and just have an
We can instead count all of the types that were elided and put that like the example below buy I honestly prefer just having
|
||
return [addSyntheticLeadingComment(factory.createKeywordTypeNode(SyntaxKind.AnyKeyword), SyntaxKind.MultiLineCommentTrivia, "elided")]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, this is discarding all the parts we already actually got to before hitting the truncation limit - shouldn't this all be something more like result.push(context.flags & NodeBuilderFlags.NoTruncation ? addSyntheticLeadingComment(factory.createKeywordTypeNode(SyntaxKind.AnyKeyword), SyntaxKind.MultiLineCommentTrivia, `... ${types.length - i} more elided ...`) : factory.createTypeReferenceNode(`... ${types.length - i} more ...`, /*typeArguments*/ undefined)); merged into the line below? This way we still get something like
out? (Part of the reason elision is a bit special here is because it endeavors to retain the last element, since elision only really makes sense in the middle.)
for the whole union, even if there's enough allotted space for a lot of the union. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Including part of types then truncating with an Arguably, adding the types might serve as documentation, but it will be incorrect as we're not including all of the types, just some of them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary for the type arithmetic? Yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed as requested but I have some concerns. I understand your point about retaining context, however, I respectfully disagree because the amount of code generated might become too large to be useful. I fear even the comment might go unnoticed. If this happens, including partial types and then truncating with any can lead to confusion and inaccuracies. I feel is more efficient to replace the entire section with i.e take a look at the test |
||
} | ||
result.push(factory.createTypeReferenceNode(`... ${types.length - i} more ...`, /*typeArguments*/ undefined)); | ||
const typeNode = typeToTypeNodeHelper(types[types.length - 1], context); | ||
if (typeNode) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,6 @@ var C = /** @class */ (function () { | |
declare class C { | ||
static D: { | ||
new (): {}; | ||
D: any; | ||
D: /*elided*/ any; | ||
}; | ||
} |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,6 +333,6 @@ declare let Anon: { | |
}; | ||
declare let OuterC: { | ||
new <out T>(): { | ||
foo(): any; | ||
foo(): /*elided*/ any; | ||
}; | ||
}; |
Uh oh!
There was an error while loading. Please reload this page.