angelozerr / tslint-language-service

TypeScript 2.2.1 plugin for tslint
MIT License
188 stars 22 forks source link

No linting rendered in VS Code 1.25.0 #76

Closed anthonynichols closed 6 years ago

anthonynichols commented 6 years ago

Updating to VS Code 1.25.0 has caused all linting information to no longer render. I checked the tsserver logs and the plugin is enabled successfully:

Info 22   [11:13:9.546] Enabling plugin tslint-language-service from candidate paths: /Users/anichols/code/test-tslint-plugin/node_modules/typescript/lib/tsserver.js/../../..
Info 23   [11:13:9.546] Loading tslint-language-service from /Users/anichols/code/test-tslint-plugin/node_modules/typescript/lib/tsserver.js/../../.. (resolved to /Users/anichols/code/test-tslint-plugin/node_modules/node_modules)
Info 24   [11:13:10.243] tslint-language-service loaded
Info 25   [11:13:10.243] Plugin validation succeded

I have created a simple example project to demonstrate: https://github.com/anthonynichols/test-tslint-plugin

I have included the tsserver.log file in that repo.

I can confirm that linting works in VS Code 1.24.1:

tslint-working

However, the same project does not work in VS Code 1.25.0:

tslint-not-working

Let me know if I can provide any additional information or help out debugging this in any way.

lingz commented 6 years ago

+1 Can report our team has the same issue

anthonynichols commented 6 years ago

@lingz I was thinking about opening an issue in the Microsoft/vscode repo as well. Thank you for taking the initiative to do so. It looks like it was closed by their bot though, so this looks to be the best place to seek a fix. Hopefully this will get noticed and addressed quickly.

Let me know if there is anything I can do to assist in getting this fixed. My (current) unfamiliarity with the plugin and the API for rendering these linting messages in VS Code are the only reasons I haven't just opened a PR with code to address this. To remedy this, and potentially add another helping hand, some documentation or a conversation explaining the landscape (or at least pointing me in the right direction to do independent study and research) for this plugin or the API that VS Code exposes to link the two together would be great.

anthonynichols commented 6 years ago

Here's an interesting finding - I was playing around with some things in the plugin code locally, mostly just stumbling around in the dark to see if I bump into anything, and I noticed const TSLINT_ERROR_CODE = 10000;. Typescript compiler options like noImplicitAny are rendered and have a code associated with them of 7006 (for example). I changed the TSLINT_ERROR_CODE to 7006 to see what would happen, and all of the tslint errors rendered for a second, and then disappeared as the tsserver.log file reported a large amount of output.

Could something be overwriting and thus disabling the tslint errors from rendering? The current configuration of the plugin seems to be working for a split second, but something about VS Code's handling of rendering the messages it is being sent by the plugin seems to be breaking between v1.24.1 and v1.25.1. I am looking through the commit messages of VS Code to see if there is anything that could be related to this issue.

lingz commented 6 years ago

Hey @anthonynichols I experience the errors all happening for a split second and dissapearing but didn;t need any changes to the code. I believe this is some kind of overwriting problem also. But I'm not familiar with either this plugin or the vscode architecture.

anthonynichols commented 6 years ago

@lingz Have you had any luck with resolving this issue yourself? I haven't been able to crack the case myself yet.

artola commented 6 years ago

@angelozerr @egamma do you have some clue why errors are shown for around 1 sec only in VSCode? I do believe that tslint-language-service is a must in the TS ecosystem, just struggling a bit to tweak the environment.

I found that the problem might be caused by function filterProblemsForDocument ... see that shortcutting the code with return failures; keeps the errors visible as desired. I will try to dive deep.

    function filterProblemsForDocument(documentPath, failures) {
      return failures; // shortcut !!!

      let normalizedPath = path.normalize(documentPath);
      // we only show diagnostics targetting this open document, some tslint rule return diagnostics for other documents/files
      let normalizedFiles = new Map();
lingz commented 6 years ago

In my environment the tslint errors are shown if there is at least one real type error. When all the errors that fail build are gone, then all the lint rules disappear also.

On Tue, 24 Jul 2018, 18:50 martin, notifications@github.com wrote:

@angelozerr https://github.com/angelozerr @egamma https://github.com/egamma do you have some clue why errors are shown for around 1 sec only in VSCode? I do believe that tslint-language-service is a must in the TS ecosystem, just struggling a bit to tweak the environment.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/angelozerr/tslint-language-service/issues/76#issuecomment-407577006, or mute the thread https://github.com/notifications/unsubscribe-auth/ADAFzQB4Kg5pyWJXw4gpmab5rAuFjpDwks5uJ6SigaJpZM4VLc1I .

artola commented 6 years ago

To continue my message above, I have narrowed the problem to getSemanticDiagnostics called with parameter fileName is fired 2 times with different casing. e.g. for a component Background.tsx

proxy.getSemanticDiagnostics = (fileName) => {
 // ...
 const tslintProblems = filterProblemsForDocument(fileName, result.failures);

@angelozerr @egamma do you have some hint? what is firing proxy.getSemanticDiagnostics and/or why of the different fileName casing?

hmil commented 6 years ago

Pretty sure this is the change that broke tslint-language-service: https://github.com/Microsoft/vscode/commit/c40a2fd096973b87e4738c2c71145fecfda73d99

According to this change, tsserver should only see lowercase filenames on a case insensitive system. However, I was able to confirm that the language service receives both lowercased and original filenames.

This is a line I got in a tsserver log file

Info 106  [20:12:45.2] request:
    {"seq":16,"type":"request","command":"geterr","arguments":{"delay":0,"files":["/Users/hmil/workspace/tslint-override-test/TestClass.ts","/users/hmil/workspace/tslint-override-test/testclass.ts"]}}

So, a good fix would be to ignore case on case insensitive systems. I do not know if typescript exposes an api to figure out whether a system is cas insensitive. Or, maybe we should just ignore case all the time. It doesn't seem likely anyone is ever going to have both a lowercase and CamelCase version of the same file in a TypeScript project.

ps: credits to @artola for pointing out the case issue.

hmil commented 6 years ago

Well that was quick. They just merged a fix in VSCode!

artola commented 6 years ago

@hmil I found that TSLint is able to handle any filename casing but reports the failures always in case sensitive (e.g. TestClass.ts). Still I do not understand why getSemanticDiagnostics is called 2 times with different filename casing. If this is produced/fixed in VSCode, we could just update VSCode (may be Insiders will get it soon) ... but in the meantime for others, might be a solution to tweak filterProblemsForDocument just the first line:

// before
let normalizedPath = path.normalize(documentPath);

// fix
let normalizedPath = path.normalize(oldLS.getProgram().getSourceFile(documentPath).fileName);

This will get the proper file casing back from the file system. And it is reporting properly the errors to me.

One more thing, is it necessary to use path.normalize in filterProblemsForDocument? (are not already normalized?)

Anyone has a better idea? Please.

hmil commented 6 years ago

Still I do not understand why getSemanticDiagnostics is called 2 times with different filename casing.

That's a bug in VSCode, they just fixed it on master. I checked and tslint-language-service now works with the master version of VSCode.

artola commented 6 years ago

@hmil Thanks a lot.

anthonynichols commented 6 years ago

This looks like it has been fixed in v1.26.0-insider 573d53815e74f8e4a52c2b6665f5ba9a92dbd341 - closing this issue. Thanks to everyone involved who looked into this!