Truncating `lineText` in `references` #51137

navya9singh · 2022-10-10T19:40:26Z

amcasey

Added some notes. Let me know if you'd like to discuss offline.

amcasey · 2022-10-10T19:42:37Z

src/server/session.ts

@@ -3522,7 +3522,26 @@ namespace ts.server {
        const scriptInfo = Debug.checkDefined(projectService.getScriptInfo(fileName));
        const span = toProtocolTextSpanWithContext(textSpan, contextSpan, scriptInfo);
        const lineSpan = scriptInfo.lineToTextSpan(span.start.line - 1);
-        const lineText = scriptInfo.getSnapshot().getText(lineSpan.start, textSpanEnd(lineSpan)).replace(/\r|\n/g, "");
+        if (lineSpan.length > 1024) {
+            var start = (span.start.offset > 500) ? (span.start.offset - 501 + lineSpan.start) : lineSpan.start;


We never use var because it has weird scoping (e.g. lineText below is visible outside the if block).

amcasey · 2022-10-10T19:43:33Z

src/server/session.ts

@@ -3522,7 +3522,26 @@ namespace ts.server {
        const scriptInfo = Debug.checkDefined(projectService.getScriptInfo(fileName));
        const span = toProtocolTextSpanWithContext(textSpan, contextSpan, scriptInfo);
        const lineSpan = scriptInfo.lineToTextSpan(span.start.line - 1);
-        const lineText = scriptInfo.getSnapshot().getText(lineSpan.start, textSpanEnd(lineSpan)).replace(/\r|\n/g, "");
+        if (lineSpan.length > 1024) {


Since 1024 is just a number I made up, it may need to be tuned. That would be easier if it were a constant.

To avoid using a magic number, it would be better to create a new variable with a proper name and store 1024 into it.

amcasey · 2022-10-10T19:44:43Z

src/server/session.ts

-        const lineText = scriptInfo.getSnapshot().getText(lineSpan.start, textSpanEnd(lineSpan)).replace(/\r|\n/g, "");
+        if (lineSpan.length > 1024) {
+            var start = (span.start.offset > 500) ? (span.start.offset - 501 + lineSpan.start) : lineSpan.start;
+            var end = (lineSpan.length -2 - span.end.offset > 500) ? (span.end.offset + 499 + lineSpan.start) : ts.textSpanEnd(lineSpan)-2; // need to ask why length is 2 more


I probably said "500", but I meant "half of the max length" (easy, if you have a constant for the max length). Also, you may discover after playing with it, that you want less text before and more text after the reference.

Usually when we subtract two from the length of a line, it's for \r and \n, but line endings vary by platform, so it's usually necessary to use a helper (we probably have one in core.ts or utilities.ts.

amcasey · 2022-10-10T19:52:59Z

src/server/session.ts

+            let tokenAtPosition = getTokenAtPosition(sourceFile, start);
+            let tokenAtPositionEnd = getTokenAtPosition(sourceFile, end - 1);
+            if (tokenAtPosition.getStart() < start) {
+                const nextToken = findNextToken(tokenAtPosition, tokenAtPosition.parent, sourceFile)?.getStart()!;


I might call this "nextTokenStart", to make it clearer that it's not a token.

Does getStart return the position before or after the leading trivia?

In here, getStart returns the start position of the token that was a result of findNextToken

But if the token looks like /*comment*/identifier, is getStart before or after the comment?

Also, the ?. followed by the ! is very questionable. Please handle the undefined explicitly.

Also, if you are calling getStart, feed in the optional sourceFile parameter.

amcasey · 2022-10-10T19:54:06Z

src/server/session.ts

+            let tokenAtPositionEnd = getTokenAtPosition(sourceFile, end - 1);
+            if (tokenAtPosition.getStart() < start) {
+                const nextToken = findNextToken(tokenAtPosition, tokenAtPosition.parent, sourceFile)?.getStart()!;
+                start = (nextToken - start) > 10 ? start : nextToken;


As this gets more polished, it might help to add comments.

amcasey · 2022-10-10T20:03:09Z

src/server/session.ts

+                const precedingToken = findPrecedingToken(end, sourceFile, tokenAtPositionEnd)?.getStart()!;
+                end = (end - precedingToken) > 10 ? end: precedingToken;
+            }               
+            lineText = scriptInfo.getSnapshot().getText(start, end).replace(/\r|\n/g, "")!;              


I wonder if we want to add "..." or something when we truncate?

Aside from the ... issue, we have a trimString (or possibly trimStringEnd) helper which you should probably use instead.

DanielRosenwasser · 2022-10-13T19:29:05Z

src/server/session.ts

+            const sourceFile = scriptInfo.getDefaultProject().getLanguageService().getProgram()!.getSourceFile(fileName)!;
+            let tokenAtPosition = getTokenAtPosition(sourceFile, start);
+            let tokenAtPositionEnd = getTokenAtPosition(sourceFile, end - 1);
+            if (tokenAtPosition.getStart() < start) {


feed in the optional sourceFile parameter to getStart.

DanielRosenwasser · 2022-10-13T19:29:46Z

src/server/session.ts

+                start = (nextToken - start) > 10 ? start : nextToken;
+            }
+            if (tokenAtPositionEnd.getEnd() > end) {
+                const precedingToken = findPrecedingToken(end, sourceFile, tokenAtPositionEnd)?.getStart()!;


Same comment with ?./! and the sourceFile argument to getStart

Hossein13M

There are conflicts which needs to be resolved for this PR to be landed

jakebailey · 2023-01-21T02:26:56Z

Closing this; #51081 was merged instead.

Truncating lineText

0e555a9

typescript-bot assigned navya9singh Oct 10, 2022

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Oct 10, 2022

navya9singh requested a review from amcasey October 10, 2022 19:40

amcasey reviewed Oct 10, 2022

View reviewed changes

DanielRosenwasser reviewed Oct 13, 2022

View reviewed changes

Hossein13M suggested changes Oct 31, 2022

View reviewed changes

jakebailey closed this Jan 21, 2023

DanielRosenwasser deleted the fix(50134) branch January 21, 2023 05:11

Truncating lineText in references #51137

Truncating lineText in references #51137

Uh oh!

Conversation

navya9singh commented Oct 10, 2022

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hossein13M left a comment

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Jan 21, 2023

Uh oh!

Uh oh!

Truncating `lineText` in `references` #51137

Truncating `lineText` in `references` #51137

DanielRosenwasser Oct 13, 2022 •

edited

Loading