Skip to content
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

Introduce truncation into node builder and symbol display part writer #24258

Merged
merged 4 commits into from
Jul 6, 2018

Conversation

weswigham
Copy link
Member

Fixes #24154

I'd like some feedback/input on some related points:

  • Are there any operations which shouldn't allow types to be truncated? (Right now everything other than declaration emit and some refactorings can be)
  • How clean does truncation need to be? While the node builder will never build any invalid nodes right now (once it estimates truncation occurs it just starts filling remaining holes with any), the display part writer is less generous and will slice in the middle of a type (though not in the middle of a display part, which is potentially better than error truncation in some ways) - this is maybe fine, but, eg, signature help is actually made up of multiple builder/writer calls, and so has multiple independent truncations occurring (see the example in the test where the constraint is truncated, but then the rest of the signature is printed and an argument type also gets truncated). Should this be fixed? Should signature help bail if it sees a truncation display part added to its part list, rather than continuing?
  • Does maximum length before truncation need to be configurable (As an LS setting or compiler option)? Should LS truncation be able to be turned off?
  • 2000 is actually a lot of characters. It works out to be around 16 solid lines of text on a wider display like mine - or exactly 25 standard 80 column lines. 25 lines of a single type is a lot of type. Should the default limit actually be even lower (we truncate types in our error messages down to 100 characters)?
  • Should whitespace and/or indentation count towards truncation length? As is, it does.

@weswigham weswigham requested review from sandersn and mhegazy May 18, 2018 21:33
goTo.marker("3");
verify.signatureHelp({
marker: "3",
text: `f<T extends "_0" | "_1" | "_2" | "_3" | "_4" | "_5" | "_6" | "_7" | "_8" | "_9" | "_10" | "_11" | "_12" | "_13" | "_14" | "_15" | "_16" | "_17" | "_18" | "_19" | "_20" | "_21" | "_22" | "_23" | "_24" | "_25" | "_26" | "_27" | "_28" | "_29" | "_30" | "_31" | "_32" | "_33" | "_34" | "_35" | "_36" | "_37" | "_38" | "_39" | "_40" | "_41" | "_42" | "_43" | "_44" | "_45" | "_46" | "_47" | "_48" | "_49" | "_50" | "_51" | "_52" | "_53" | "_54" | "_55" | "_56" | "_57" | "_58" | "_59" | "_60" | "_61" | "_62" | "_63" | "_64" | "_65" | "_66" | "_67" | "_68" | "_69" | "_70" | "_71" | "_72" | "_73" | "_74" | "_75" | "_76" | "_77" | "_78" | "_79" | "_80" | "_81" | "_82" | "_83" | "_84" | "_85" | "_86" | "_87" | "_88" | "_89" | "_90" | "_91" | "_92" | "_93" | "_94" | "_95" | "_96" | "_97" | "_98" | "_99" | "_100" | "_101" | "_102" | "_103" | "_104" | "_105" | "_106" | "_107" | "_108" | "_109" | "_110" | "_111" | "_112" | "_113" | "_114" | "_115" | "_116" | "_117" | "_118" | "_119" | "_120" | "_121" | "_122" | "_123" | "_124" | "_125" | "_126" | "_127" | "_128" | "_129" | "_130" | "_131" | "_132" | "_133" | "_134" | "_135" | "_136" | "_137" | "_138" | "_139" | "_140" | "_141" | "_142" | "_143" | "_144" | "_145" | "_146" | "_147" | "_148" | "_149" | "_150" | "_151" | "_152" | "_153" | "_154" | "_155" | "_156" | "_157" | "_158" | "_159" | "_160" | "_161" | "_162" | "_163" | "_164" | "_165" | "_166" | "_167" | "_168" | "_169" | "_170" | "_171" | "_172" | "_173" | "_174" | "_175" | "_176" | "_177" | "_178" | "_179" | "_180" | "_181" | "_182" | "_183" | "_184" | "_185" | "_186" | "_187" | "_188" | "_189" | "_190" | "_191" | "_192" | "_193" | "_194" | "_195" | "_196" | "_197" | "_198" | "_199" | "_200" | "_201" | "_202" | "_203" | "_204" | "_205" | "_206" | "_207" | "_208" | "_209" | "_210" | "_211" | "_212" | "_213" | "_214" | "_215" | "_216" | "_217" | "_218" | "_219" | "_220" | "_221" | "_222" | "_223" | "_224" | "_225" | "_226" | "_227" | "_228" | "_229" | "_230" | "_231" | "_232" | "_233" ...(s: T, x: Exclude<"_0", T> | Exclude<"_1", T> | Exclude<"_2", T> | Exclude<"_3", T> | Exclude<"_4", T> | Exclude<"_5", T> | Exclude<"_6", T> | Exclude<"_7", T> | Exclude<"_8", T> | Exclude<"_9", T> | Exclude<"_10", T> | Exclude<"_11", T> | Exclude<"_12", T> | Exclude<"_13", T> | Exclude<"_14", T> | Exclude<"_15", T> | Exclude<"_16", T> | Exclude<"_17", T> | Exclude<"_18", T> | Exclude<"_19", T> | Exclude<"_20", T> | Exclude<"_21", T> | Exclude<"_22", T> | Exclude<"_23", T> | Exclude<"_24", T> | Exclude<"_25", T> | Exclude<"_26", T> | Exclude<"_27", T> | Exclude<"_28", T> | Exclude<"_29", T> | Exclude<"_30", T> | Exclude<"_31", T> | Exclude<"_32", T> | Exclude<"_33", T> | Exclude<"_34", T> | Exclude<"_35", T> | Exclude<"_36", T> | Exclude<"_37", T> | Exclude<"_38", T> | Exclude<"_39", T> | Exclude<"_40", T> | Exclude<"_41", T> | Exclude<"_42", T> | Exclude<"_43", T> | Exclude<"_44", T> | Exclude<"_45", T> | Exclude<"_46", T> | Exclude<"_47", T> | Exclude<"_48", T> | Exclude<"_49", T> | Exclude<"_50", T> | Exclude<"_51", T> | Exclude<"_52", T> | Exclude<"_53", T> | Exclude<"_54", T> | Exclude<"_55", T> | Exclude<"_56", T> | Exclude<"_57", T> | Exclude<"_58", T> | Exclude<"_59", T> | Exclude<"_60", T> | Exclude<"_61", T> | Exclude<"_62", T> | Exclude<"_63", T> | Exclude<"_64", T> | Exclude<"_65", T> | Exclude<"_66", T> | Exclude<"_67", T> | Exclude<"_68", T> | Exclude<"_69", T> | Exclude<"_70", T> | Exclude<"_71", T> | Exclude<"_72", T> | Exclude<"_73", T> | Exclude<"_74", T> | Exclude<"_75", T> | Exclude<"_76", T> | Exclude<"_77", T> | Exclude<"_78", T> | Exclude<"_79", T> | Exclude<"_80", T> | Exclude<"_81", T> | Exclude<"_82", T> | Exclude<"_83", T> | Exclude<"_84", T> | Exclude<"_85", T> | Exclude<"_86", T> | Exclude<"_87", T> | Exclude<"_88", T> | Exclude<"_89", T> | Exclude<"_90", T> | Exclude<"_91", T> | Exclude<"_92", T> | Exclude<"_93", T> | Exclude<"_94", T> | Exclude<"_95", T> | Exclude<"_96", T> | Exclude<"_97", T> | Exclude<"_98", T> | Exclude<"_99", T> | Exclude< ..., y: string): void`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re your question about multiple truncations: they're really bad. The length is so long that multiple truncations don't appear near each other, and the truncation start looking like some kind of Typescript-specific syntax.

That said, I have no idea how to make things better besides some vague ideas that sound like a lot of work.

@@ -3077,64 +3082,81 @@ namespace ts {
return undefined;
}

if (type.flags & TypeFlags.Any) {
if (type.flags & TypeFlags.Any || (!(context.flags & NodeBuilderFlags.NoTruncation) && context.approximateLength > (context.tracker.maximumApproximateLength || 2000))) {
context.approximateLength += 3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking the max length state at each level of the tree walk is a start, but I think a complete solution would also be able to abort at any level of the tree walk too. Specifically, globally abort and then truncate instead of aborting and truncating locally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why the code here doesn't do that, actually. Why don't the short-circuits in witeKind et al implement a global truncation?

Copy link
Member Author

@weswigham weswigham May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're global within the context of a single writer, however signature help composes multiple writer calls to build the full display...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no :(

@@ -1435,14 +1435,25 @@ namespace ts {

const displayPartWriter = getDisplayPartWriter();
function getDisplayPartWriter(): DisplayPartsSymbolWriter {
const maximumApproximateLength = 2000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that seems a lot.. not sure what is a good number.. but 100 seems slightly too short.. so may be something in the range of 150 - 200..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

240 because that's 3 80 character lines?

@mhegazy
Copy link
Contributor

mhegazy commented Jul 5, 2018

the approach here is similar to that that we are doing today but giving more room, and avoiding doing the work if we know we are going to through away.. so that OK.. but what i had in mind is to be a bit smarter.. e.g. for a type with too many properties, limit the number of properties, e.g. { a: number, .... z: number } | undefined instead of { a: number, b: number, .... . Similarly, something with multiple levels of nesting, cutting the depth instead of cutting the breadth, so { a: { ... }, b: number, c: string } instead of { a: { nested1: { nested2: { nested3: ...

@weswigham
Copy link
Member Author

e.g. for a type with too many properties,

Ehhh..., the thing is, 20 x: number is really short, but one x: A<B<C<D<E<F<G<H<I<...>>>>> is really long (and there's not really a great way to shorten generic instantiations, AFAIK). A cutoff would be arbitrary and wouldn't have a great correlation with text length.

@weswigham
Copy link
Member Author

Hmmmm I can probably make truncation a bit smarter in a similar way though, at the cost of adhering to the max length less strictly.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 6, 2018

I do not think we have a hard limit on the number of characters, it is ok if we are a few chars off as long as 1. we have a readable type, and 2. we are not doing a lot of type-2-node work that we will through away

@weswigham weswigham force-pushed the node-builder-ls-truncation branch from fb09626 to 52aa4bc Compare July 6, 2018 20:00
@weswigham
Copy link
Member Author

@mhegazy we now have fancy truncation:

type DeeplyMapped = {
    _0: {
        _0: ["_0", "_0", 0, 0];
        _1: ["_0", "_1", 0, 1];
        _2: ["_0", "_2", 0, 2];
        _3: ["_0", "_3", 0, 3];
        _4: ["_0", "_4", 0, 4];
        _5: ["_0", "_5", 0, 5];
        _6: ["_0", "_6", 0, 6];
        _7: ["_0", "_7", 0, 7];
        ... 491 more ...;
        _499: [...];
    };
    ... 498 more ...;
    _499: {
        ...;
    };
}

There's also a hard limit of 10x the default truncation length (of 160) where it'll just stop and append ... still; just in case any really long structures occur which can't be fancily truncated (ie, a 1000 character identifier).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Truncate quickinfo LS requests
3 participants