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

[typescript-language-features] Region-based semantic diagnostics for TypeScript #208713

Merged
merged 14 commits into from
Jun 19, 2024

Conversation

gabritto
Copy link
Member

@gabritto gabritto marked this pull request as draft March 27, 2024 03:31
@mjbvz mjbvz added this to the April 2024 milestone Apr 8, 2024
@mjbvz mjbvz modified the milestones: April 2024, May 2024 Apr 23, 2024
@mjbvz mjbvz modified the milestones: May 2024, June 2024 May 30, 2024
@gabritto
Copy link
Member Author

gabritto commented May 30, 2024

I found an issue when testing this PR with the TS Server counterpart on microsoft/TypeScript#57842. The issue is that vscode sends a geterr request to TS Server, but before all diagnostics for all open files are computed, vscode cancels that geterr request and sends a getApplicableRefactors request. Then, after getApplicableRefactors is finished, it sends a geterr request again (because geterr requests are queued when they're cancelled before finishing), and once again that request is cancelled before finishing and a getApplicableRefactors request is sent to TS Server.

The TS Server log for this looks like the following:

Info 15441[12:48:34.661] request:
    {
      "seq": 3822,
      "type": "request",
      "command": "geterr",
      "arguments": {
        "delay": 0,
        "files": [
          {
            "file": "c:\\Users\\gabrielaa\\code\\main\\Typescript\\src\\compiler\\checker.ts",
            "ranges": [
              {
                "startLine": 22458,
                "startOffset": 1,
                "endLine": 22488,
                "endOffset": 61
              }
            ]
          },
          {
            "file": "c:\\Users\\gabrielaa\\code\\main\\Typescript\\src\\compiler\\types.ts",
            "ranges": [
              {
                "startLine": 907,
                "startOffset": 1,
                "endLine": 938,
                "endOffset": 69
              }
            ]
          }
        ]
      }
    }
Perf 15442[12:48:34.661] 3822::geterr: async elapsed time (in milliseconds) 0.2594
Info 15443[12:48:34.676] event:
    {"seq":0,"type":"event","event":"syntaxDiag","body":{"file":"c:/Users/gabrielaa/code/main/Typescript/src/compiler/checker.ts","diagnostics":[],"duration":0.0208}}
Info 15444[12:48:34.694] event:
    {"seq":0,"type":"event","event":"regionSemanticDiag","body":{"file":"c:/Users/gabrielaa/code/main/Typescript/src/compiler/checker.ts","diagnostics":[],"spans":[{"start":{"line":22312,"offset":10},"end":{"line":22523,"offset":10}}],"duration":17.9772}}
Info 15445[12:48:35.033] event:
    {"seq":0,"type":"event","event":"requestCompleted","body":{"request_seq":3822}}
Info 15446[12:48:35.033] request:
    {
      "seq": 3823,
      "type": "request",
      "command": "getApplicableRefactors",
      "arguments": {
        "file": "c:\\Users\\gabrielaa\\code\\main\\Typescript\\src\\compiler\\checker.ts",
        "startLine": 22465,
        "startOffset": 49,
        "endLine": 22465,
        "endOffset": 49,
        "triggerReason": "implicit",
        "includeInteractiveActions": true
      }
    }
Perf 15447[12:48:35.034] 3823::getApplicableRefactors: elapsed time (in milliseconds) 0.9841
Info 15448[12:48:35.034] response:
    {"seq":0,"type":"response","command":"getApplicableRefactors","request_seq":3823,"success":true,"body":[{"name":"Convert export","description":"Convert default export to named export","actions":[{"name":"Convert default export to named export","description":"Convert default export to named export","kind":"refactor.rewrite.export.named","notApplicableReason":"Could not find export statement"},{"name":"Convert named export to default export","description":"Convert named export to default export","kind":"refactor.rewrite.export.default","notApplicableReason":"Could not find export statement"}]},{"name":"Convert import","description":"Convert namespace import to named imports","actions":[{"name":"Convert namespace import to named imports","description":"Convert namespace import to named imports","kind":"refactor.rewrite.import.named","notApplicableReason":"Selection is not an import declaration."}]},{"name":"Convert import","description":"Convert named imports to default import","actions":[{"name":"Convert named imports to default import","description":"Convert named imports to default import","kind":"refactor.rewrite.import.default","notApplicableReason":"Selection is not an import declaration."}]},{"name":"Convert import","description":"Convert named imports to namespace import","actions":[{"name":"Convert named imports to namespace import","description":"Convert named imports to namespace import","kind":"refactor.rewrite.import.namespace","notApplicableReason":"Selection is not an import declaration."}]},{"name":"Extract type","description":"Extract type","actions":[{"name":"Extract to typedef","description":"Extract to typedef","kind":"refactor.extract.typedef","notApplicableReason":"Selection is not a valid type node"},{"name":"Extract to type alias","description":"Extract to type alias","kind":"refactor.extract.type","notApplicableReason":"Selection is not a valid type node"},{"name":"Extract to interface","description":"Extract to interface","kind":"refactor.extract.interface","notApplicableReason":"Selection is not a valid type node"}]},{"name":"Move to a new file","description":"Move to a new file","actions":[{"name":"Move to a new file","description":"Move to a new file","kind":"refactor.move.newFile","range":{"start":{"line":1443,"offset":1},"end":{"line":51577,"offset":2}}}]},{"name":"Add or remove braces in an arrow function","description":"Add or remove braces in an arrow function","actions":[{"name":"Add braces to arrow function","description":"Add braces to arrow function","kind":"refactor.rewrite.arrow.braces.add","notApplicableReason":"Containing function is not an arrow function"},{"name":"Remove braces from arrow function","description":"Remove braces from arrow function","kind":"refactor.rewrite.arrow.braces.remove","notApplicableReason":"Containing function is not an arrow function"}]},{"name":"Convert to template string","description":"Convert to template string","actions":[{"name":"Convert to template string","description":"Convert to template string","kind":"refactor.rewrite.string","notApplicableReason":"Can only convert string concatenations and string literals"}]},{"name":"Extract Symbol","description":"Extract function","actions":[{"name":"Extract Function","description":"Extract function","kind":"refactor.extract.function","notApplicableReason":"Cannot extract empty range."}]},{"name":"Extract Symbol","description":"Extract constant","actions":[{"name":"Extract Constant","description":"Extract constant","kind":"refactor.extract.constant","notApplicableReason":"Cannot extract empty range."}]},{"name":"Generate 'get' and 'set' accessors","description":"Generate 'get' and 'set' accessors","actions":[{"name":"Generate 'get' and 'set' accessors","description":"Generate 'get' and 'set' accessors","kind":"refactor.rewrite.property.generateAccessors","notApplicableReason":"Could not find property for which to generate accessor"}]},{"name":"Infer function return type","description":"Infer function return type","actions":[{"name":"Infer function return type","description":"Infer function return type","kind":"refactor.rewrite.function.returnType","notApplicableReason":"Return type must be inferred from a function"}]}]}
Info 15449[12:48:35.226] request:
    {
      "seq": 3824,
      "type": "request",
      "command": "geterr",
      "arguments": {
        "delay": 0,
        "files": [
          {
            "file": "c:\\Users\\gabrielaa\\code\\main\\Typescript\\src\\compiler\\checker.ts",
            "ranges": [
              {
                "startLine": 22458,
                "startOffset": 1,
                "endLine": 22488,
                "endOffset": 61
              }
            ]
          },
          {
            "file": "c:\\Users\\gabrielaa\\code\\main\\Typescript\\src\\compiler\\types.ts",
            "ranges": [
              {
                "startLine": 907,
                "startOffset": 1,
                "endLine": 938,
                "endOffset": 69
              }
            ]
          }
        ]
      }
    }
Perf 15450[12:48:35.226] 3824::geterr: async elapsed time (in milliseconds) 0.2108
Info 15451[12:48:35.242] event:
    {"seq":0,"type":"event","event":"syntaxDiag","body":{"file":"c:/Users/gabrielaa/code/main/Typescript/src/compiler/checker.ts","diagnostics":[],"duration":0.0131}}
Info 15452[12:48:35.260] event:
    {"seq":0,"type":"event","event":"regionSemanticDiag","body":{"file":"c:/Users/gabrielaa/code/main/Typescript/src/compiler/checker.ts","diagnostics":[],"spans":[{"start":{"line":22312,"offset":10},"end":{"line":22523,"offset":10}}],"duration":18.5655}}
Info 15453[12:48:35.596] event:
    {"seq":0,"type":"event","event":"requestCompleted","body":{"request_seq":3824}}
Info 15454[12:48:35.597] request:
    {
      "seq": 3825,
      "type": "request",
      "command": "getApplicableRefactors",
      "arguments": {
        "file": "c:\\Users\\gabrielaa\\code\\main\\Typescript\\src\\compiler\\checker.ts",
        "startLine": 22465,
        "startOffset": 49,
        "endLine": 22465,
        "endOffset": 49,
        "triggerReason": "implicit",
        "includeInteractiveActions": true
      }
    }
Perf 15455[12:48:35.598] 3825::getApplicableRefactors: elapsed time (in milliseconds) 0.9566
Info 15456[12:48:35.598] response:
    {"seq":0,"type":"response","command":"getApplicableRefactors","request_seq":3825,"success":true,"body":[{"name":"Convert export","description":"Convert default export to named export","actions":[{"name":"Convert default export to named export","description":"Convert default export to named export","kind":"refactor.rewrite.export.named","notApplicableReason":"Could not find export statement"},{"name":"Convert named export to default export","description":"Convert named export to default export","kind":"refactor.rewrite.export.default","notApplicableReason":"Could not find export statement"}]},{"name":"Convert import","description":"Convert namespace import to named imports","actions":[{"name":"Convert namespace import to named imports","description":"Convert namespace import to named imports","kind":"refactor.rewrite.import.named","notApplicableReason":"Selection is not an import declaration."}]},{"name":"Convert import","description":"Convert named imports to default import","actions":[{"name":"Convert named imports to default import","description":"Convert named imports to default import","kind":"refactor.rewrite.import.default","notApplicableReason":"Selection is not an import declaration."}]},{"name":"Convert import","description":"Convert named imports to namespace import","actions":[{"name":"Convert named imports to namespace import","description":"Convert named imports to namespace import","kind":"refactor.rewrite.import.namespace","notApplicableReason":"Selection is not an import declaration."}]},{"name":"Extract type","description":"Extract type","actions":[{"name":"Extract to typedef","description":"Extract to typedef","kind":"refactor.extract.typedef","notApplicableReason":"Selection is not a valid type node"},{"name":"Extract to type alias","description":"Extract to type alias","kind":"refactor.extract.type","notApplicableReason":"Selection is not a valid type node"},{"name":"Extract to interface","description":"Extract to interface","kind":"refactor.extract.interface","notApplicableReason":"Selection is not a valid type node"}]},{"name":"Move to a new file","description":"Move to a new file","actions":[{"name":"Move to a new file","description":"Move to a new file","kind":"refactor.move.newFile","range":{"start":{"line":1443,"offset":1},"end":{"line":51577,"offset":2}}}]},{"name":"Add or remove braces in an arrow function","description":"Add or remove braces in an arrow function","actions":[{"name":"Add braces to arrow function","description":"Add braces to arrow function","kind":"refactor.rewrite.arrow.braces.add","notApplicableReason":"Containing function is not an arrow function"},{"name":"Remove braces from arrow function","description":"Remove braces from arrow function","kind":"refactor.rewrite.arrow.braces.remove","notApplicableReason":"Containing function is not an arrow function"}]},{"name":"Convert to template string","description":"Convert to template string","actions":[{"name":"Convert to template string","description":"Convert to template string","kind":"refactor.rewrite.string","notApplicableReason":"Can only convert string concatenations and string literals"}]},{"name":"Extract Symbol","description":"Extract function","actions":[{"name":"Extract Function","description":"Extract function","kind":"refactor.extract.function","notApplicableReason":"Cannot extract empty range."}]},{"name":"Extract Symbol","description":"Extract constant","actions":[{"name":"Extract Constant","description":"Extract constant","kind":"refactor.extract.constant","notApplicableReason":"Cannot extract empty range."}]},{"name":"Generate 'get' and 'set' accessors","description":"Generate 'get' and 'set' accessors","actions":[{"name":"Generate 'get' and 'set' accessors","description":"Generate 'get' and 'set' accessors","kind":"refactor.rewrite.property.generateAccessors","notApplicableReason":"Could not find property for which to generate accessor"}]},{"name":"Infer function return type","description":"Infer function return type","actions":[{"name":"Infer function return type","description":"Infer function return type","kind":"refactor.rewrite.function.returnType","notApplicableReason":"Return type must be inferred from a function"}]}]}

and this keeps going on indefinitely.

Steps to reproduce:

  1. Build, run and open a local version of vscode that uses the code in this PR.

  2. In vscode settings, enable the option TypeScript > Tsserver: Enable Region Diagnostics:
    image

  3. In vscode settings, set TS Server logging to verbose:
    image

  4. Build a local version of TypeScript with the code in Region-based semantic diagnostics TypeScript#57842.

  5. In vscode, open the TypeScript repository (https://github.com/microsoft/TypeScript/).

  6. Run npm ci on the terminal on the TS repository to make sure dependencies are working (I can't be sure this matters or not for reproing) and there are no errors.

  7. Set vscode to use the local version of TypeScript from step 4.

  8. Open the files src/compiler/checker.ts and src/compiler/types.ts side by side, such that both are visible (I tried reproing this with only one file open and couldn't).

  9. Go to line 917 (Reported = 1 << 2) on file src/compiler/types.ts and comment it out and move cursor to anywhere on src/compiler/checker.ts.

  10. At this point, you should be able to open the TS Server logs on a different vscode instance and look for "geterr". In the logs, you should see the repeating pattern mentioned above (if you keep updating the file, you'll notice the pattern go on forever). Also, on the vscode "Problems" tab, there should be no listed errors on file checker.ts (or an incomplete list of errors, depending on what region of that file is currently visible in the editor).

@gabritto
Copy link
Member Author

Some more things I noticed about the problem above:

  • I can't repro it if I only have src/compiler/checker.ts open on vscode.
  • I can't repro on a dummy project simpler than TypeScript. It might be that the problem happens because it takes over 3s to do a full semantic check of src/compilerchecker.ts.
@gabritto
Copy link
Member Author

Updates on the problem: I found exactly why/where we keep requesting getApplicableRefactors: class CodeModelOracle is ultimately responsible. It registers a listener to the markerChange event here, and this listener eventually calls getCodeActions, which eventually results in calling provideCodeActions in the TypeScript extension and which interrupts the ongoing geterr request.

So what happens is roughly this:

  • vscode sends geterr request for files checker.ts, types.ts.
  • tsserver starts checking file checker.ts and sends back syntax diagnostics event and region semantic diagnostics event
  • vscode updates diagnostics for checker.ts as it receives those events
  • updating diagnostics triggers the markerChange event
  • the CodeModelOracle listener to markerChange calls getCodeActions, causing the registered language providers to try and provide code actions, including the TS extension
  • the TS extension attempts to provide refactor code actions, and to do that it interrupts the ongoing geterr request and sends a getApplicableRefactors request to tsserver
  • since geterr was interrupted, vscode geterr request is sent again to tsserver, and the cycle restarts
@gabritto
Copy link
Member Author

The problem I described above seems to be solved by #215389.

@gabritto gabritto marked this pull request as ready for review June 13, 2024 23:23
Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks! Will be in the next VS Code insiders

@mjbvz mjbvz merged commit 878af07 into microsoft:main Jun 19, 2024
6 checks passed
bricefriha pushed a commit to bricefriha/vscode that referenced this pull request Jun 26, 2024
…TypeScript (microsoft#208713)

* WIP

* invalidate diagnostics in range

* check whether should use region based diagnostics

* add ts-expect-errors

* make region opt off by default

* bump to expected 5.6

* update comments to refer to 5.6

* make region diagnostics on by default for insiders
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants