Skip invalid completion check immediately after newline #55061

suica · 2023-07-18T13:27:02Z

Fixes #54729

Demo:

tests/cases/fourslash/server/completions04.ts

suica · 2023-07-18T15:40:17Z

@microsoft-github-policy-service agree

svipas · 2023-11-07T19:38:30Z

@sandersn could we expect this to be in TypeScript 5.3 or at least in 5.4? This is really needed for DX.

iisaduan · 2023-11-08T23:12:24Z

@typescript-bot pack this

gabritto · 2023-11-10T19:12:16Z

@typescript-bot pack this

typescript-bot · 2023-11-10T19:12:19Z

Heya @gabritto, I've started to run the tarball bundle task on this PR at 6eb4ff3. You can monitor the build here.

typescript-bot · 2023-11-10T19:14:16Z

Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/158553/artifacts?artifactName=tgz&fileId=39BD9E9D5446EBD2B72515AE8DB4BF764C12AF5B973FEDCDD9CD2BDC8E28119502&fileName=/typescript-5.4.0-insiders.20231110.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.4.0-pr-55061-8".;

suica · 2023-11-11T13:02:01Z

cool

zardoy · 2023-11-11T17:03:13Z

absolutely insane

gabritto · 2023-11-15T20:51:36Z

@typescript-bot user test tsserver
@typescript-bot test tsserver top100
@typescript-bot perf test this

typescript-bot · 2023-11-15T20:51:39Z

Heya @gabritto, I've started to run the diff-based user code test suite (tsserver) on this PR at 6eb4ff3. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-11-15T20:51:39Z

Heya @gabritto, I've started to run the regular perf test suite on this PR at 6eb4ff3. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-11-15T20:51:39Z

Heya @gabritto, I've started to run the diff-based top-repos suite (tsserver) on this PR at 6eb4ff3. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-11-15T21:05:05Z

@gabritto Here are the results of running the user test suite comparing main and refs/pull/55061/merge:

Everything looks good!

gabritto · 2023-11-15T21:13:23Z

src/services/completions.ts

    function isCompletionListBlocker(contextToken: Node): boolean {
        const start = timestamp();
        const result = isInStringOrRegularExpressionOrTemplateLiteral(contextToken) ||
-            isSolelyIdentifierDefinitionLocation(contextToken) ||
+            // GH#54729 ignore this case when current position is in a newline


I think, if we wanted to be more precise in when we allow completions, this check of isInDifferentLineWithContextToken should be done inside isSolelyIdentifierDefinitionLocation, and should be done case-by-case, because I don't think we want to allow completions in a newline for every possible case isSolelyIdentifierDefinitionLocation returns true.

For instance, isSolelyIdentifierDefinitionLocation returns true on the following position:

var [x, y |

It doesn't look like you'd actually want completions at that position.

Thank you! Tried to fix this, @gabritto plz take a look at the new code :)

iisaduan

Thank you for the contribution, and I apologize for the spam while trying to get the bot to work!

The change in general seems good, but we should probably use the check to alter isSolelyIdentifierDefinitionLocation instead of patching over its result.

src/services/completions.ts

iisaduan · 2023-11-15T20:38:02Z

src/services/completions.ts

+            // GH#54729 ignore this case when current position is in a newline
+            (isSolelyIdentifierDefinitionLocation(contextToken) && !isInDifferentLineWithContextToken(contextToken, position)) ||


Suggested change

// GH#54729 ignore this case when current position is in a newline

(isSolelyIdentifierDefinitionLocation(contextToken) && !isInDifferentLineWithContextToken(contextToken, position)) ||

isSolelyIdentifierDefinitionLocation(contextToken) ||

IMO it makes more sense to put the isInDifferentLineWithContextToken check into the return statement of isSolelyIdentifierDefinitionLocation, so we don't unblock too many cases.

Thank you! gonna fix it

Fixed, and now the logic looks less messy

src/services/completions.ts

typescript-bot · 2023-11-15T21:18:40Z

@gabritto
The results of the perf run you requested are in!

Here they are:

Compiler

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{Angular - node (v18.15.0, x64)}
_{Memory used}	_{295,159k (± 0.01%)}	_{295,185k (± 0.02%)}	_~	_295,118k	_295,244k	_{p=0.378 n=6}
_{Parse Time}	_{2.64s (± 0.28%)}	_{2.64s (± 0.44%)}	_~	_2.62s	_2.65s	_{p=0.734 n=6}
_{Bind Time}	_{0.82s (± 0.00%)}	_{0.83s (± 0.99%)}	_~	_0.82s	_0.84s	_{p=0.073 n=6}
_{Check Time}	_{8.05s (± 0.19%)}	_{8.04s (± 0.36%)}	_~	_8.00s	_8.07s	_{p=0.405 n=6}
_{Emit Time}	_{7.08s (± 0.27%)}	_{7.08s (± 0.27%)}	_~	_7.06s	_7.11s	_{p=1.000 n=6}
_{Total Time}	_{18.59s (± 0.17%)}	_{18.58s (± 0.18%)}	_~	_18.53s	_18.62s	_{p=0.628 n=6}
_{Compiler-Unions - node (v18.15.0, x64)}
_{Memory used}	_{193,629k (± 1.67%)}	_{192,664k (± 1.59%)}	_~	_190,677k	_196,736k	_{p=1.000 n=6}
_{Parse Time}	_{1.36s (± 1.38%)}	_{1.34s (± 1.19%)}	_~	_1.31s	_1.35s	_{p=0.318 n=6}
_{Bind Time}	_{0.72s (± 0.00%)}	_{0.72s (± 0.00%)}	_~	_0.72s	_0.72s	_{p=1.000 n=6}
_{Check Time}	_{9.16s (± 0.41%)}	_{9.20s (± 0.39%)}	_~	_9.15s	_9.25s	_{p=0.063 n=6}
_{Emit Time}	_{2.62s (± 0.75%)}	_{2.62s (± 0.39%)}	_~	_2.61s	_2.63s	_{p=1.000 n=6}
_{Total Time}	_{13.85s (± 0.34%)}	_{13.88s (± 0.26%)}	_~	_13.83s	_13.93s	_{p=0.228 n=6}
_{Monaco - node (v18.15.0, x64)}
_{Memory used}	_{347,353k (± 0.00%)}	_{347,352k (± 0.00%)}	_~	_347,328k	_347,369k	_{p=0.810 n=6}
_{Parse Time}	_{2.46s (± 0.31%)}	_{2.46s (± 0.50%)}	_~	_2.44s	_2.47s	_{p=0.315 n=6}
_{Bind Time}	_{0.93s (± 0.59%)}	_{0.93s (± 0.56%)}	_~	_0.92s	_0.93s	_{p=0.640 n=6}
_{Check Time}	_{6.92s (± 0.46%)}	_{6.91s (± 0.44%)}	_~	_6.89s	_6.97s	_{p=0.685 n=6}
_{Emit Time}	_{4.07s (± 0.72%)}	_{4.05s (± 0.70%)}	_~	_4.02s	_4.10s	_{p=0.222 n=6}
_{Total Time}	_{14.37s (± 0.31%)}	_{14.34s (± 0.31%)}	_~	_14.30s	_14.42s	_{p=0.296 n=6}
_{TFS - node (v18.15.0, x64)}
_{Memory used}	_{302,613k (± 0.00%)}	_{302,621k (± 0.01%)}	_~	_302,590k	_302,643k	_{p=0.297 n=6}
_{Parse Time}	_{2.01s (± 1.27%)}	_{2.01s (± 1.08%)}	_~	_1.98s	_2.04s	_{p=0.809 n=6}
_{Bind Time}	_{1.01s (± 1.32%)}	_{1.00s (± 1.09%)}	_~	_0.99s	_1.02s	_{p=0.209 n=6}
_{Check Time}	_{6.26s (± 0.41%)}	_{6.28s (± 0.57%)}	_~	_6.22s	_6.32s	_{p=0.372 n=6}
_{Emit Time}	_{3.58s (± 0.33%)}	_{3.58s (± 0.43%)}	_~	_3.56s	_3.60s	_{p=0.869 n=6}
_{Total Time}	_{12.86s (± 0.32%)}	_{12.86s (± 0.35%)}	_~	_12.81s	_12.91s	_{p=1.000 n=6}
_{material-ui - node (v18.15.0, x64)}
_{Memory used}	_{470,552k (± 0.01%)}	_{470,558k (± 0.00%)}	_~	_470,543k	_470,572k	_{p=0.470 n=6}
_{Parse Time}	_{2.56s (± 0.21%)}	_{2.57s (± 1.08%)}	_~	_2.54s	_2.62s	_{p=0.866 n=6}
_{Bind Time}	_{0.99s (± 1.18%)}	_{0.99s (± 0.90%)}	_~	_0.98s	_1.00s	_{p=0.933 n=6}
_{Check Time}	_{16.66s (± 0.33%)}	_{16.64s (± 0.32%)}	_~	_16.55s	_16.70s	_{p=0.574 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{20.22s (± 0.29%)}	_{20.19s (± 0.30%)}	_~	_20.08s	_20.23s	_{p=0.808 n=6}
_{xstate - node (v18.15.0, x64)}
_{Memory used}	_{512,810k (± 0.01%)}	_{512,834k (± 0.01%)}	_~	_512,773k	_512,911k	_{p=0.470 n=6}
_{Parse Time}	_{3.28s (± 0.42%)}	_{3.27s (± 0.27%)}	_~	_3.26s	_3.28s	_{p=0.562 n=6}
_{Bind Time}	_{1.54s (± 0.26%)}	_{1.54s (± 0.33%)}	_~	_1.54s	_1.55s	_{p=0.595 n=6}
_{Check Time}	_{2.87s (± 0.84%)}	_{2.86s (± 0.57%)}	_~	_2.84s	_2.88s	_{p=0.746 n=6}
_{Emit Time}	_{0.08s (± 4.99%)}	_{0.08s (± 6.19%)}	_~	_0.08s	_0.09s	_{p=0.595 n=6}
_{Total Time}	_{7.76s (± 0.23%)}	_{7.76s (± 0.21%)}	_~	_7.74s	_7.78s	_{p=0.683 n=6}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

Angular - node (v18.15.0, x64)
Compiler-Unions - node (v18.15.0, x64)
Monaco - node (v18.15.0, x64)
TFS - node (v18.15.0, x64)
material-ui - node (v18.15.0, x64)
xstate - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

tsserver

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{Compiler-UnionsTSServer - node (v18.15.0, x64)}
_{Req 1 - updateOpen}	_{2,365ms (± 0.63%)}	_{2,373ms (± 1.16%)}	_~	_2,342ms	_2,412ms	_{p=0.689 n=6}
_{Req 2 - geterr}	_{5,389ms (± 1.51%)}	_{5,376ms (± 1.73%)}	_~	_5,303ms	_5,498ms	_{p=1.000 n=6}
_{Req 3 - references}	_{326ms (± 0.74%)}	_{329ms (± 1.14%)}	_~	_325ms	_334ms	_{p=0.195 n=6}
_{Req 4 - navto}	_{277ms (± 1.09%)}	_{277ms (± 0.80%)}	_~	_274ms	_279ms	_{p=0.934 n=6}
_{Req 5 - completionInfo count}	_{1,356 (± 0.00%)}	_{1,356 (± 0.00%)}	_~	_1,356	_1,356	_{p=1.000 n=6}
_{Req 5 - completionInfo}	_{85ms (± 7.04%)}	_{81ms (± 8.68%)}	_~	_75ms	_90ms	_{p=0.366 n=6}
_{CompilerTSServer - node (v18.15.0, x64)}
_{Req 1 - updateOpen}	_{2,489ms (± 0.50%)}	_{2,505ms (± 0.81%)}	_~	_2,467ms	_2,522ms	_{p=0.128 n=6}
_{Req 2 - geterr}	_{4,103ms (± 1.87%)}	_{4,081ms (± 2.05%)}	_~	_4,018ms	_4,201ms	_{p=0.810 n=6}
_{Req 3 - references}	_{338ms (± 1.60%)}	_{341ms (± 1.58%)}	_~	_333ms	_345ms	_{p=0.744 n=6}
_{Req 4 - navto}	_{283ms (± 0.30%)}	_{283ms (± 0.45%)}	_~	_281ms	_284ms	_{p=0.445 n=6}
_{Req 5 - completionInfo count}	_{1,518 (± 0.00%)}	_{1,518 (± 0.00%)}	_~	_1,518	_1,518	_{p=1.000 n=6}
_{Req 5 - completionInfo}	_{79ms (± 7.32%)}	_{86ms (± 6.57%)}	_{🔻+8ms (+ 9.51%)}	_79ms	_90ms	_{p=0.037 n=6}
_{xstateTSServer - node (v18.15.0, x64)}
_{Req 1 - updateOpen}	_{2,605ms (± 0.48%)}	_{2,595ms (± 0.45%)}	_~	_2,575ms	_2,609ms	_{p=0.336 n=6}
_{Req 2 - geterr}	_{1,729ms (± 1.90%)}	_{1,711ms (± 2.26%)}	_~	_1,664ms	_1,752ms	_{p=0.378 n=6}
_{Req 3 - references}	_{115ms (± 6.44%)}	_{122ms (± 3.19%)}	_~	_117ms	_126ms	_{p=0.076 n=6}
_{Req 4 - navto}	_{366ms (± 0.51%)}	_{366ms (± 0.54%)}	_~	_364ms	_369ms	_{p=0.625 n=6}
_{Req 5 - completionInfo count}	_{2,073 (± 0.00%)}	_{2,073 (± 0.00%)}	_~	_2,073	_2,073	_{p=1.000 n=6}
_{Req 5 - completionInfo}	_{311ms (± 1.75%)}	_{304ms (± 2.60%)}	_~	_298ms	_318ms	_{p=0.092 n=6}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

CompilerTSServer - node (v18.15.0, x64)
Compiler-UnionsTSServer - node (v18.15.0, x64)
xstateTSServer - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

Startup

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{tsc-startup - node (v18.15.0, x64)}
_{Execution time}	_{152.61ms (± 0.20%)}	_{152.50ms (± 0.18%)}	_{-0.11ms (- 0.07%)}	_151.45ms	_155.65ms	_{p=0.000 n=600}
_{tsserver-startup - node (v18.15.0, x64)}
_{Execution time}	_{227.41ms (± 0.15%)}	_{227.36ms (± 0.16%)}	_~	_226.13ms	_233.49ms	_{p=0.053 n=600}
_{tsserverlibrary-startup - node (v18.15.0, x64)}
_{Execution time}	_{229.32ms (± 0.16%)}	_{229.36ms (± 0.17%)}	_~	_227.64ms	_235.40ms	_{p=0.263 n=600}
_{typescript-startup - node (v18.15.0, x64)}
_{Execution time}	_{228.96ms (± 0.15%)}	_{228.98ms (± 0.19%)}	_~	_227.51ms	_235.45ms	_{p=0.460 n=600}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

tsc-startup - node (v18.15.0, x64)
tsserver-startup - node (v18.15.0, x64)
tsserverlibrary-startup - node (v18.15.0, x64)
typescript-startup - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

Developer Information:

Download Benchmarks

typescript-bot · 2023-11-15T22:35:41Z

@gabritto Here are the results of running the top-repos suite comparing main and refs/pull/55061/merge:

Everything looks good!

Co-authored-by: Isabel Duan <isabelduan@microsoft.com>

tests/cases/fourslash/completionAfterNewline.ts

Co-authored-by: Isabel Duan <isabelduan@microsoft.com>

iisaduan

Looks good!

Note: this change only adds the newline after variable declarations

svipas · 2023-11-21T12:42:42Z

@iisaduan Thank you for merging it. And, @suica for fixing the issue. 🎉

fix(54729): skip invalid completion location check in newline

47090fd

typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Jul 18, 2023

suica added 2 commits July 18, 2023 22:20

fix: fix failed cases

a6b5427

test: add test case

2a0ff06

zardoy reviewed Jul 18, 2023

View reviewed changes

tests/cases/fourslash/server/completions04.ts Outdated Show resolved Hide resolved

suica added 4 commits July 18, 2023 23:24

fix: fix failed test

2129319

test: add one more case

ffdcfe4

test: modify test case

0588091

refactor: clean code

deff25b

suica added 2 commits July 18, 2023 23:43

chore: fix lint error

b91464d

chore: clean test code

6eb4ff3

suica marked this pull request as ready for review July 18, 2023 15:46

sandersn requested review from iisaduan and navya9singh July 28, 2023 22:39

sandersn assigned navya9singh Jul 28, 2023

This comment was marked as duplicate.

Sign in to view

gabritto reviewed Nov 15, 2023

View reviewed changes

iisaduan requested changes Nov 15, 2023

View reviewed changes

suica and others added 8 commits November 16, 2023 10:48

Update src/services/completions.ts

a2ae899

Co-authored-by: Isabel Duan <isabelduan@microsoft.com>

Update src/services/completions.ts

dbbdae5

Co-authored-by: Isabel Duan <isabelduan@microsoft.com>

Merge branch 'microsoft:main' into completion

992783f

refactor

380dd4b

fix

1de8beb

rename variable

c6accf5

fmt

6b6d53f

reword tests

e44eedd

suica requested review from iisaduan and gabritto November 16, 2023 07:10

iisaduan reviewed Nov 16, 2023

View reviewed changes

tests/cases/fourslash/completionAfterNewline.ts Outdated Show resolved Hide resolved

Update tests/cases/fourslash/completionAfterNewline.ts

6359bc7

Co-authored-by: Isabel Duan <isabelduan@microsoft.com>

suica requested a review from iisaduan November 17, 2023 05:55

iisaduan approved these changes Nov 17, 2023

View reviewed changes

iisaduan merged commit ffc21e5 into microsoft:main Nov 20, 2023
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip invalid completion check immediately after newline #55061

Skip invalid completion check immediately after newline #55061

suica commented Jul 18, 2023 •

edited

Loading

suica commented Jul 18, 2023

svipas commented Nov 7, 2023

iisaduan commented Nov 8, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

gabritto commented Nov 10, 2023

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

suica commented Nov 11, 2023

zardoy commented Nov 11, 2023

This comment was marked as duplicate.

gabritto commented Nov 15, 2023

typescript-bot commented Nov 15, 2023 •

edited

Loading

typescript-bot commented Nov 15, 2023 •

edited

Loading

typescript-bot commented Nov 15, 2023 •

edited

Loading

typescript-bot commented Nov 15, 2023

gabritto Nov 15, 2023 •

edited

Loading

suica Nov 16, 2023

iisaduan left a comment •

edited

Loading

iisaduan Nov 15, 2023

suica Nov 16, 2023

suica Nov 16, 2023

typescript-bot commented Nov 15, 2023

Compiler

tsserver

Startup

typescript-bot commented Nov 15, 2023

iisaduan left a comment

svipas commented Nov 21, 2023

		// GH#54729 ignore this case when current position is in a newline
		(isSolelyIdentifierDefinitionLocation(contextToken) && !isInDifferentLineWithContextToken(contextToken, position)) \|\|

Skip invalid completion check immediately after newline #55061

Skip invalid completion check immediately after newline #55061

Conversation

suica commented Jul 18, 2023 • edited Loading

suica commented Jul 18, 2023

svipas commented Nov 7, 2023

iisaduan commented Nov 8, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

gabritto commented Nov 10, 2023

typescript-bot commented Nov 10, 2023 • edited Loading

typescript-bot commented Nov 10, 2023 • edited Loading

suica commented Nov 11, 2023

zardoy commented Nov 11, 2023

This comment was marked as duplicate.

gabritto commented Nov 15, 2023

typescript-bot commented Nov 15, 2023 • edited Loading

typescript-bot commented Nov 15, 2023 • edited Loading

typescript-bot commented Nov 15, 2023 • edited Loading

typescript-bot commented Nov 15, 2023

gabritto Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

suica Nov 16, 2023

Choose a reason for hiding this comment

iisaduan left a comment • edited Loading

Choose a reason for hiding this comment

iisaduan Nov 15, 2023

Choose a reason for hiding this comment

suica Nov 16, 2023

Choose a reason for hiding this comment

suica Nov 16, 2023

Choose a reason for hiding this comment

typescript-bot commented Nov 15, 2023

Compiler

tsserver

Startup

typescript-bot commented Nov 15, 2023

iisaduan left a comment

Choose a reason for hiding this comment

svipas commented Nov 21, 2023

suica commented Jul 18, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 15, 2023 •

edited

Loading

typescript-bot commented Nov 15, 2023 •

edited

Loading

typescript-bot commented Nov 15, 2023 •

edited

Loading

gabritto Nov 15, 2023 •

edited

Loading

iisaduan left a comment •

edited

Loading