sourcegraph / javascript-typescript-langserver

JavaScript and TypeScript code intelligence through the Language Server Protocol
https://sourcegraph.com
Apache License 2.0
793 stars 72 forks source link

Diagnostics range sometimes invalid #482

Open ul opened 6 years ago

ul commented 6 years ago

Steps to reproduce

(paths are replaced with ... for brevity)

  1. Create an empty project with package.json
  2. Add index.js file with content f][;
  3. Run javascript-typescript-langserver and initialize it with rootUri of that project (i.e. {"jsonrpc":"2.0","method":"initialize","params":{"capabilities":{"textDocument":{},"workspace":{}},"processId":94866,"rootPath":"...","rootUri":"...","trace":"off"},"id":0})
  4. Wait for response and send didOpen notification: {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"languageId":"javascript","text":"f][;\n","uri":"...","version":1}}}
  5. Wait for diagnostics to be published

Expected result

All ranges in published diagnostics are valid.

Actual result

One of diagnostics has range {"start":{"character":0,"line":0},"end":{"character":0,"line":0}} which is invalid.

 {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"...","diagnostics":[{"range":{"start":{"line":0,"character":1},"end":{"line":0,"character":2}},"message":"';' expected.","severity":1,"code":1005,"source":"ts"},{"range":{"start":{"line":0,"character":3},"end":{"line":0,"character":4}},"message":"Expression or comma expected.","severity":1,"code":1137,"source":"ts"},{"range":{"start":{"character":0,"line":0},"end":{"character":0,"line":0}},"message":"']' expected.","severity":1,"code":1005,"source":"ts"}]}}
felixfbecker commented 6 years ago

For JSON parsing, I think we just use JSON.parse(), so if the JSON file is invalid we don't have an exact range for the syntax error. But I don't see where in the spec it says that empty ranges are invalid?

ul commented 6 years ago

Problem is not in range emptiness, but that spec defines following convention:

If you want to specify a range that contains a line including the line ending character(s) then use an end position denoting the start of the next line.

This way 0.0-0.0 range defines range from the first character of line 0 to the EOL of line -1, which is non-sense. I tend to agree that it could be more vagueness of spec or just my misinterpretation of it than javascript-typescript-langserver's bug, feel free to close issue in that case.

ul commented 6 years ago

Also, it's not a JSON file, it's 'index.js' JavaScript file.

felixfbecker commented 6 years ago

Hmm, if it's a JavaScript file then it is probably not even a range we produce, but the TypeScript API. What does tsserver (native VS Code TS support) return?

ul commented 6 years ago
11:35 ~/t/inline-diag-js-test 130 ❯ tsserver 
Content-Length: 75

{"seq":0,"type":"event","event":"typingsInstallerPid","body":{"pid":2093}}
{"seq":1,"type":"quickinfo","command":"open","arguments":{"file":"/Users/rprakapchuk/tmp/inline-diag-js-test/index.js"}}
Content-Length: 347

{"seq":0,"type":"event","event":"setTypings","body":{"projectName":"/dev/null/inferredProject1*","typeAcquisition":{"enable":true,"include":[],"exclude":[]},"compilerOptions":{"target":1,"jsx":1,"allowNonTsExtensions":true,"allowJs":true,"noEmitForJsFiles":true,"maxNodeModuleJsDepth":2},"typings":[],"unresolvedImports":[],"kind":"action::set"}}
Content-Length: 140

{"seq":0,"type":"event","event":"projectsUpdatedInBackground","body":{"openFiles":["/Users/rprakapchuk/tmp/inline-diag-js-test/index.js"]}}
Content-Length: 490

{"seq":0,"type":"event","event":"syntaxDiag","body":{"file":"/Users/rprakapchuk/tmp/inline-diag-js-test/index.js","diagnostics":[{"start":{"line":1,"offset":2},"end":{"line":1,"offset":3},"text":"';' expected.","code":1005,"category":"error"},{"start":{"line":1,"offset":4},"end":{"line":1,"offset":5},"text":"Expression or comma expected.","code":1137,"category":"error"},{"start":{"line":1,"offset":6},"end":{"line":1,"offset":6},"text":"']' expected.","code":1005,"category":"error"}]}}
Content-Length: 135

{"seq":0,"type":"event","event":"semanticDiag","body":{"file":"/Users/rprakapchuk/tmp/inline-diag-js-test/index.js","diagnostics":[]}}
Content-Length: 137

{"seq":0,"type":"event","event":"suggestionDiag","body":{"file":"/Users/rprakapchuk/tmp/inline-diag-js-test/index.js","diagnostics":[]}}