Added typeToTypeNode with truncation #59332

armanio123 · 2024-07-17T21:18:53Z

Make completions and formatting handle truncation gracefully. In this PR, when truncation is reported instead tsserver will replace the type with any type. This should also solved the issue mentioned by @weswigham in this comment #58719 (comment) about force truncation after 1,000,000 characters.

andrewbranch · 2024-07-18T00:16:20Z

src/services/codefixes/helpers.ts

+    return parameters.map((parameter, i) =>
+        parameter.type
+            && (!signature.thisParameter && i === 0)
+            && typeToTypeNode(checker, checker.getParameterType(signature, i - (signature.thisParameter ? 1 : 0)), enclosingDeclaration, flags, context).isTruncated


I don't think I understand why we need to round-trip from a TypeNode (parameter.type) to a Type back to a TypeNode 🤔

The only method I have found to make the checker re-serialize is by using checker.typeToTypeNode. Since parameter.type is TypeNode, I first get the type, then the TypeNode after identifying truncation.

Hope this makes sense.

But why do we need to reserialize?

I think the problem is that we're calling checker.signatureToSignatureDeclaration on this line: https://github.com/microsoft/TypeScript/pull/59332/files#diff-880eafae481c815c3af893123092cc0d93305284d8e6d2200638e7d546123b6cR414.

That means that the types get converted to type nodes as part of that call to signatureToSignatureDeclaration, and if we don't have direct control here of where the type nodes are being created, we can't replace the truncated ones.

I think a better approach to avoid serializing parameter types twice (and any other types that show up in a signature) would be to directly change the truncation behavior of the checker functions that convert things to nodes, e.g. typeToTypeNode, etc. So instead of inserting ... and similar, it could insert any, for instance.
I think we could have a new context flag to control this, and set this new flag everywhere the relevant checker functions are being called in this file and others modified by this PR.

You're correct @gabritto. The first time serializing is during checker.signatureToSignatureDeclaration. Replacing truncation there is going to be complicated becuase reporting truncation happens on high level.

I do agree that is better to replace the NoTruncation behavior with using an any node. The challenge I see there is that by the time we have identified truncation, we have already serialized a bunch of nodes. We must then replace all of the with a single any node. Otherwise, we'll end up with a huge union with with an any in the second to last node.

I have not analyzed if that's feasible, but I can certainly look into it if it's promising.

As per @gabritto comment, I've looked into adding a new flag that replaces the truncation behavior for replacing it with an any keyword. So far, the change looks good and very promising. The only problem is that we no longer have space for the NodeBuilderFlag to add a flag; I'm starting to look into that.

I'm slightly concerned around any showing up in some of these places in case a user ever asks "where did this any come from?"

One alternative might be to create a node like {/*...*/} or [/*...*/] which retains that comment, or even [...elided].

Another approach might be to introduce a special ... type node that is recognized by the parser. It could still be an error, but gracefully parsed and interpreted as an errorType in the checker.

Maybe the concern with the any isn't as big a deal as I'm making it out to be, but I'm wondering what others think.

I don’t think we should insert a syntax error. I think adding an inline comment like /* elided */ or /* error */ to an any is fine, maybe an improvement over a silently introduced any. I think a new special error type node is an interesting idea and I’m not against it; however, remember that this bug comes from fuzzing, not user reports or even telemetry, so I’m not sure how much effort should go into polishing what seems to be a rare edge case.

I kind of like the idea of prepending a comment like /* elided */ or something similar just to indicate to the user that this node got truncated and has been transformed to any. Arguably this might be confusing to the user, but at least something to search.

I have not search in telemetry to figure out the recurrence of this bug in release and I don't think we will get many user reports either because this causes completions to not be displayed. Maybe users didn't notice or care to report. Nonetheless, the error-deltas recurrently finds these errors.

I have updated the PR to instead elide in case of truncation. This doesn't add a new flag as we discussed, because I think it would be unnecessary. Plus. we already handle it like that when creating declarations, so this is consistent with that behavior.

Ping @weswigham as there's some discussion about it privately.

codecov-commenter · 2024-07-18T00:54:31Z

Codecov Report

Attention: Patch coverage is 97.95918% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (main@972e9a7). Learn more about missing BASE report.

Files	Patch %	Lines
src/services/codefixes/helpers.ts	97.82%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #59332   +/-   ##
=======================================
  Coverage        ?   94.07%           
=======================================
  Files           ?      587           
  Lines           ?   300872           
  Branches        ?     5080           
=======================================
  Hits            ?   283044           
  Misses          ?    12748           
  Partials        ?     5080

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/baselines/reference/emitClassExpressionInDeclarationFile.js

weswigham

I think createTypeNodesFromResolvedType and the later half of mapToTypeNodes have references to checkTruncationLength that don't have a style switch like this (since they don't go through createElidedInformationPlaceholder), still. They need to be a bit more creative than an any to be grammatical, though, since they're not type node positions.

typeToString also has an emergency hard cutoff that slices the output and adds a trailing ... in the event the smart truncation fails - not sure if you want to adjust that, or if you even can without introducing a hang (you definitely have to have a cutoff for typeToString at some point to avoid a hang from sending 5MB of type text across the protocol). I don't think typeToString's output ever gets formatted, so it having non-grammatical output for that function specifically is probably fine.

Otherwise, this seems OK (sans duplicated comment in that one test - but that should be avoidable) - dunno if an /*elided*/ comment is sufficient feedback for users, but if it works 🤷‍♀️

src/compiler/factory/nodeFactory.ts

armanio123 · 2024-08-09T22:45:12Z

@weswigham I don't understand how to fix the createTypeNodesFromResolvedType. We want to report an error when creating a declaration for a type correct?

i.e the code from test hugeDeclarationOutputGetsTruncatedWithError.ts shows the error The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.

// @declaration: true
type props = "a" | "b" | "c" | "d" | "e" | "f" | "g" | "h" | "i" | "j" | "k" | "l" | "m" | "n" | "o" | "p" | "q" | "r" | "s" | "t" | "u" | "v" | "w" | "x" | "y" | "z";

type manyprops = `${props}${props}`;

export const c = [null as any as {[K in manyprops]: {[K2 in manyprops]: `${K}.${K2}`}}][0];

because c resolves to:

>c : { aa: { aa: "aa.aa"; ... 527 more ...; zz: { ...; }; }

we can instead resolve c to something like any or { [key: string]: any } but seems to me like this might be incorrect

weswigham · 2024-08-15T19:56:58Z

We want to report an error when creating a declaration for a type correct?

Truncations already report a truncation error (via reportTruncationError) which the caller may/may not ignore - if the caller is ignoring it, but has indicated they need syntactically valid output by setting NoTruncation, we should probably emit something other than ... 527 more ..., yes. I wouldn't go with an index signature, though - maybe... something like /*... 527 more ...*/ as a trailing comment to the prior member?

armanio123 · 2024-08-15T23:01:23Z

yes. I wouldn't go with an index signature, though - maybe... something like /... 527 more .../ as a trailing comment to the prior member?

Do you find feasible to do c: /* elided */ any instead of commenting the additional ones? I think that we can check for truncation happening on createTypeNodeFromObjectType and use the same "elided" logic. I actually prefer to have the same behavior when NoTruncation is happening.

armanio123 · 2024-08-15T23:21:22Z

To your comment about typeToString, doesn't seem to me like we need to fix this method. Unless I'm understanding it wrong, the result of typeToString is a string to be used in errors, display parts, or other places that servers as documentation. Having being truncated by using ... sounds more usefull.

weswigham · 2024-08-16T00:05:04Z

I actually prefer to have the same behavior when NoTruncation is happening.

Yeah, I generally do, too, that's why usually it all goes through the createElidedInformationPlaceholder helper usually - but that location has some extra formatting to include the number of skipped elements in the output (since that location isn't skipping a type node, but rather is skipping some number of property declarations), so doesn't. Seems like we aughta retain that? I think it looks a bit nicer to have the count in that position anyway, and it's not like a random type node is syntactically valid there, so you need some kind of special handling. To be clear, what I'm saying is that rather than

>c : { aa: { aa: "aa.aa"; ... 527 more ...; zz: { ...; }; }

when NoTruncation is set, we could emit

>c : { aa: { aa: "aa.aa"; /*... 527 more ...*/ zz: { /*elided*/ }; }

attaching the elision text as a (trailing) comment to the last successful node, rather than inserting the syntactically invalid node or a bogus property/signature.

armanio123 · 2024-08-16T00:57:49Z

Changed as requested. I still have to figure out how to handle the zz: { /*elided*/ } scenario.

.vscode/tasks.json

src/compiler/factory/nodeFactory.ts

jakebailey · 2024-08-16T23:51:41Z

src/compiler/checker.ts

@@ -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) {


Am I confused or is this truncating things more when the flag is enabled?

It is not more per se, but it is replacing the previous values with an any

We kinda have 3 states: when NoTruncation is disabled, we truncate up to defaultMaximumTruncationLength, if NoTruncation is enabled it truncates up to noTruncationMaximumTruncationLength, anything after that is an any.

A ... ${types.length - i} more elided ... comment would be more in-line with the non-grammatical node this replaces when NoTruncation is set.

If we decide to remove all of the types and just have an any it will not make sense to have this message as the code will look like:

foo(bar: /*... 516 more elided ...*/ any): /*elided*/ any { throw new Error("Method not implemented."); } get bar(): /*... 516 more elided ...*/ any { throw new Error("Method not implemented."); } set bar(value: /*... 516 more elided ...*/ any) { throw new Error("Method not implemented."); } baz<V extends /*... 516 more elided ...*/ any>(value: V): V { throw new Error("Method not implemented."); }

We can instead count all of the types that were elided and put that like the example below buy I honestly prefer just having /*elided*/ for something short and descriptive enough IMO.

foo(bar: /*... 800 elided ...*/ any): /*... 800 elided ...*/ any { throw new Error("Method not implemented."); }

typescript-bot · 2024-08-19T22:33:06Z

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

weswigham

It'd also be nice to add a test case that actually hits the createNotEmittedTypeElement call, just so we know it looks like what we expect. That should just be a matter of a quickfix test like the existing one where the truncation limit gets hit within an object literal type, afaik.

weswigham · 2024-08-20T15:15:43Z

src/compiler/checker.ts

@@ -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) {


A ... ${types.length - i} more elided ... comment would be more in-line with the non-grammatical node this replaces when NoTruncation is set.

weswigham · 2024-08-20T15:22:43Z

src/compiler/checker.ts

@@ -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) {
+                            return [addSyntheticLeadingComment(factory.createKeywordTypeNode(SyntaxKind.AnyKeyword), SyntaxKind.MultiLineCommentTrivia, "elided")];


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

A | B | C | /* ... 6 more elided ... */ any | undefined

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.)
As written, this is just going to output

/* elided */ any

for the whole union, even if there's enough allotted space for a lot of the union.

Including part of types then truncating with an any seems to me like it would be incorrect and unnecessary, even for the last type. Instead, I would argue that if we are truncating and putting any anyway, we might as well remove the whole thing and replace it for an any.

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.

Unnecessary for the type arithmetic? Yes. A | B | any is just any. Unnecessary for the human that has to go and fill in the blank after the quickfix runs? Probably not! The more context we retain, the easier it is for the human that has to go and fix this fix's output with what's supposed to go there!

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 any to maintain clarity and correctness. This way, we avoid the risk of misinterpretation and ensure the code remains manageable.

i.e take a look at the test codeFixClassImplementInterfaceNoTruncation.ts in the latest change, finding where the parameter type ends is already complicated. A user noticing the comment, or the introduction of any seems likely to be missed.

armanio123 · 2024-08-21T23:10:20Z

It'd also be nice to add a test case that actually hits the createNotEmittedTypeElement

I don't seem to find a way to create a declaration file with an error like truncation, so I've been testing it locally by removing the error itself. There's an ongoing thread about this here #54305

This reverts commit 90dade9.

weswigham

👍 I like the consistency between quick info and the fix output now.

cc @rbuckton just to be aware of the NotEmittedTypeElement node kind and it being public, in case he can think of anything we should do for a new node kind that we've just forgotten (surely we'd have some kind of typesafe thing yelling at us, though, right? 😅).

i.e take a look at the test codeFixClassImplementInterfaceNoTruncation.ts in the latest change, finding where the parameter type ends is already complicated.

Yeah, IMO, that's the point of setting NoTruncation - you get the full fat, complex output - as much as we can give. It's possible we may want a middleground in the future, where we want the syntactic validity but not the extra long truncation length the flag also implies, but we can consider that separately. We may just wanna make the truncation length easily configurable in the future (though we need to remember that will affect node caching).

I fear even the comment might go unnoticed.

Actually, that's a good idea for another new feature/followup - maybe we should issue suggestions/errors on explicitly written any (or unknown) in unions with other members, since we know they trivially collapse the union to nothing! What could you possibly intend when you write A | B | any, anyway?

I don't seem to find a way to create a declaration file with an error like truncation, so I've been testing it locally by removing the error itself

I was thinking of triggering it via a quickfix, since that's what the new test was doing - you have to tune the length of the expansion-generated nodes pretty precisely to get it to truncate inside the explicit object you want truncated, though (you need just enough length to be below the limit, and then the object literal we want to truncate within).

rbuckton · 2024-08-24T00:22:29Z

src/compiler/checker.ts

@@ -7054,6 +7055,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

            function createTypeNodesFromResolvedType(resolvedType: ResolvedType): TypeElement[] | undefined {
                if (checkTruncationLength(context)) {
+                    if (context.flags & NodeBuilderFlags.NoTruncation) {


Is there a test that shows what this looks like?

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.

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?

After deeper analysis, the file hugeDeclarationOutputGetsTruncatedWithError.types access this code.

Check line 18, for the last property zz. It changes from "zz": { ...; } to zz: { };. This is a "type" tests, during emit the "elided" comment gets added: zz: { /*elided*/ };

Why don't we add elided in type tests?

I was working on it. Added.

Just to confirm, you're saying that in *.types files we will write "zz": { ...; } but in emit we will write zz: { /*elided*/ };? The thing I was interested in is whether or not we should do the latter for both, but maybe that's not what you were describing?

No, I mean, before this change files in *.types files and during emit we write "zz": { ...; }.

After this change, *.types files and emit write "zz": { }, and .d.ts files "zz": { /*elided*/ } respectively.

See the test codeFixClassImplementInterfaceNoTruncationProperties.ts for the emit example. hugeDeclarationOutputGetsTruncatedWithError.types shows the *.types change.

armanio123 requested review from DanielRosenwasser, andrewbranch and gabritto July 17, 2024 21:18

armanio123 mentioned this pull request Jul 17, 2024

[ServerErrors][TypeScript] 5.6.0-dev.20240707 vs 5.5.3 #59168

Closed

andrewbranch reviewed Jul 18, 2024

View reviewed changes

armanio123 force-pushed the FixNoTruncationRootCause branch from c05195a to 4d598b0 Compare August 8, 2024 23:16

jakebailey reviewed Aug 8, 2024

View reviewed changes

tests/baselines/reference/emitClassExpressionInDeclarationFile.js Outdated Show resolved Hide resolved

weswigham reviewed Aug 8, 2024

View reviewed changes

jakebailey reviewed Aug 9, 2024

View reviewed changes

src/compiler/factory/nodeFactory.ts Outdated Show resolved Hide resolved

jakebailey mentioned this pull request Aug 12, 2024

Simplify mergeEmitNode #59613

Closed

rbuckton mentioned this pull request Aug 15, 2024

Do not overwrite original pointer when indirectly points to location #59647

Merged

Armando Aguirre Sepulveda added 5 commits August 16, 2024 14:27

Elide truncated typenodes

f6e323c

Fixed duplicate elided comment

9092688

Change appendIfUnique for overwriting

44568f7

Handle truncation in createTypeNodesFromResolvedType

c1b1d40

dprint...

52c2ff1

armanio123 force-pushed the FixNoTruncationRootCause branch from 8dc147e to 52c2ff1 Compare August 16, 2024 21:30

Missing baseline

05431a6

jakebailey reviewed Aug 16, 2024

View reviewed changes

.vscode/tasks.json Outdated Show resolved Hide resolved

jakebailey reviewed Aug 16, 2024

View reviewed changes

src/compiler/factory/nodeFactory.ts Outdated Show resolved Hide resolved

Revert typos

5810eab

jakebailey reviewed Aug 16, 2024

View reviewed changes

Added NotEmittedTypeElement

5891f32

This was referenced Aug 19, 2024

[ServerErrors][TypeScript] 5.6.0-dev.20240811 vs 5.5.4 #59596

Closed

[ServerErrors][TypeScript] 5.6.0-dev.20240818 vs 5.5.4 #59678

Closed

weswigham reviewed Aug 20, 2024

View reviewed changes

Changed elided comment

90dade9

Armando Aguirre Sepulveda added 2 commits August 21, 2024 16:17

Revert "Changed elided comment"

3b44569

This reverts commit 90dade9.

Dont remove types

2cbb9f3

weswigham approved these changes Aug 23, 2024

View reviewed changes

rbuckton reviewed Aug 24, 2024

View reviewed changes

Added elided test

ac4ba37

jakebailey approved these changes Aug 30, 2024

View reviewed changes

rbuckton approved these changes Aug 30, 2024

View reviewed changes

armanio123 mentioned this pull request Aug 30, 2024

[ServerErrors][TypeScript] 5.7.0-dev.20240825 vs 5.5.4 #59752

Closed

armanio123 merged commit 6260f7a into microsoft:main Aug 30, 2024
32 checks passed

Added typeToTypeNode with truncation #59332

Added typeToTypeNode with truncation #59332

Conversation

armanio123 commented Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 18, 2024

Codecov Report

weswigham left a comment

Choose a reason for hiding this comment

armanio123 commented Aug 9, 2024

weswigham commented Aug 15, 2024

armanio123 commented Aug 15, 2024

armanio123 commented Aug 15, 2024

weswigham commented Aug 16, 2024 • edited Loading

armanio123 commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanio123 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

typescript-bot commented Aug 19, 2024

weswigham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanio123 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

weswigham Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanio123 commented Aug 21, 2024 • edited Loading

weswigham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanio123 Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanio123 Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanio123 commented Jul 17, 2024 •

edited

Loading

DanielRosenwasser Jul 25, 2024 •

edited

Loading

weswigham commented Aug 16, 2024 •

edited

Loading

armanio123 Aug 21, 2024 •

edited

Loading

armanio123 Aug 21, 2024 •

edited

Loading

weswigham Aug 22, 2024 •

edited

Loading

armanio123 commented Aug 21, 2024 •

edited

Loading

armanio123 Aug 26, 2024 •

edited

Loading

armanio123 Aug 28, 2024 •

edited

Loading