Add support for diagnostic severities

[egamma]

TypeScript has added compiler options for lint level checks like noUnusedParameters or noUnusedLocals in tsconfig.json (which is goodness). Today when such an option is enabled then the corresponding issues are reported by TypeScript in the same way as semantic or syntax errors. There is no notion of severity.

Background

We are using tslint to develop VS Code and we have used tslint rules to detect unused locals. TSLint rule failures are reported in VS code as warnings and we had the setup that lint style issue are shown as warnings and the typescript issues (syntax, semantic) are shown as errors. Now that typescript added more lint style checks we no longer get the distinction when developing between lint level warnings and semantic errors. The situation has become worse with tslint 4.0, there tslint has started to deprecate rules which are covered by TypeScript compiler options. This makes good sense, but it means we are now starting to see 'unused locals' reported as errors and no longer as warnings as we did before.

Suggestion

Support that the user can define in the tsconfig.json whether a check enabled by compiler option should be reported as error or warning.

eslint supports to configure the severity of an option. Here is an example the eslint documentation:

{
    "rules": {
        "eqeqeq": "off",
        "curly": "warn",
        "quotes": ["error", "double"]
    }
}

// CC @waderyan @mjbvz

[kkovalevskiy]

Also it'd be nice to have ability to disable some checks for certain places in the code by corresponding comments as it's already implemented in tslint - /* tslint:disable */, etc (https://palantir.github.io/tslint/usage/rule-flags/)

[n9niwas]

+1 would be nice to have that.

[mhegazy]

also discussed in #6802

[donaldpipowitch]

I'd love to see this feature in the roadmap. Could this become part of 2.3?

I personally need this for warnings for

• noImplicitAny
• noUnusedLocals
• noUnusedParameters

[mhegazy]

Also referenced as the underlying issue in #15550

[jez9999]

This feature seems to be going very slowly - nearly a year and still only in the suggestion phase?

Personally I would love to see this feature. "Unreachable code detected" should absolutely be a warning, not an error. During development I regularly return before some code I temporarily want to ignore, and it is by design. It is not an error.

[jez9999]

Just in case anyone was having the annoyance with the "unreachable code" error I was, this error can be completely disabled with "allowUnreachableCode": true in your tsconfig.json.

[mhegazy]

Based on the disccussion in #18894, a new flag --treatWraningsAsErrorrs will be added, defaults to true. The servirty of the style checks done by the compiler will all be set to Warrning. here is the list of configurations to control these messages :

• --noUnusedLocals
• --noUnusedParameters
• --noImplicitReturns
• --noFallthroughCasesInSwitch
• --allowUnusedLabels
• --allowUnreachableCode

[alexeagle]

I agree with Erich (as one should!) about the problem of these "lint" options appearing as errors in the editor. It's distracting, and I think worse, since they are indistinguishable from type errors it teaches new developers on a large team that TypeScript is really demanding.

However, the workflow outside of the editor is negatively impacted by the presence of these "lint" options. As we have observed at Google (and we have a paper on this about to be published in CACM, let me know if you'd like an advance copy), there is no good answer when a developer is presented with a wall of warnings.

One approach is to roll up the shirtsleeves and declare "I'm getting warning-clean". This is supported by the proposed --treatWarningsAsErrors. However, unless this is baked into the development process carefully (eg. reserve half a day before each release), no one regularly goes to this effort. Furthermore, there is a survivor bias: warnings older than one or two releases are not actually a problem, if the software is working correctly for users. So we found that this use case is generally a waste of time, and I think it is embarked upon by a developer whose personality drives them to dislike the warnings piled up by their co-workers, not by good engineering practice.

The other approach taken most of the time is to ignore the "existing" warnings. Here is where this proposal falls flat, IMO. What we really want in Warnings that differs from Errors is that we permit legacy occurrences but discourage new ones. I don't think this proposal does enough to allow tooling to support this workflow. This is because the warnings and errors are presented together in the command-line output. There is no way for a developer or for tooling to present only the newly introduced warnings.

TSLint already produces warnings, of course. Thoughtful tooling can do something different with the output/exit code of tsc versus the ones from tslint. In Google, we present tslint results only during the code review process, either by the developer running something akin to gulp lint, or by the code review tool automatically running the linter, akin to GitHub status checks appearing on the PR. In these workflows, we suppress any warnings on unchanged code.

Ideally, I wish we could move these options to tslint, because the mental model for toolchain developers is much simpler if one tool is used for correctness, and another for possible bugs and style nits (and another for formatting) since the way these tools should be run is very different, in particular when they should be run. Is it still possible to backtrack and move these flags to tslint?

Assuming that's not possible, and TypeScript will continue to support "lint" checks, could we give this a bit more thought before landing the feature?

[mhegazy]

Ideally, I wish we could move these options to tslint, because the mental model for toolchain developers is much simpler if one tool is used for correctness, and another for possible bugs and style nits (and another for formatting) since the way these tools should be run is very different, in particular when they should be run. Is it still possible to backtrack and move these flags to tslint?

I think this is the core of the issue. Most of the core team tends to agree with this as well. In hind sight, we should have not added such lint-level checks to the compiler and left them to be implemented in tslint. I do not think we can take them out now.. so this does not leave us with many options rely.

[alexeagle]

Have you thought through what would be required to back it out? Would it help if I can find and guide a contributor to port these checks to tslint?

[weswigham]

Have you thought through what would be required to back it out? Would it help if I can find and guide a contributor to port these checks to tslint?

Tslint has been actively deprecating and removing duplicates of the functionality provided by these flags.

[alexeagle]

That hasn't worked out. We escalated that tslint needed to keep noUnusedLocals palantir/tslint#2256 (of course the reasoning given at the time was that we needed the warning behavior). What is the vision for whether a check belongs first-party in the compiler or in a separate tool? It seems like the tslint team has been moving in the direction of de-duplicating checks added to TypeScript as a reaction to what happened upstream, not because someone made a solid proposal why it should work this way.

…

On Mon, Oct 16, 2017 at 1:01 PM Wesley Wigham ***@***.***> wrote: Have you thought through what would be required to back it out? Would it help if I can find and guide a contributor to port these checks to tslint? Tslint has been actively deprecating and removing <palantir/tslint#661> duplicates of the functionality provided by these flags. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#13408 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC5I4j6RKMbjsrdbtVmDMp43utz_Ydiks5ss4vHgaJpZM4LgcLC> .

[jez9999]

For that matter, why not move all linting functionality into TypeScript as configurable warnings/errors/off? Linting tools are usually there to optionally stop certain practices in languages that already allow that stuff. As TypeScript is parsing everything anyway during transpilation, one of the services is could also offer is linting.

You would of course want to build in an extension mechanism to allow people to add their own linting rules.

[weswigham]

@jez9999 We looked into that once.

[donaldpipowitch]

@andy-ms or @mhegazy I tried to get the new suggestions via the Node API and especially would like to apply the suggestions to autofix problems (e.g. remove unused vars automatically).

I normally get diagnostics like this:

// @ts-check
const {
  createProgram,
  readConfigFile,
  parseJsonConfigFileContent,
  sys
} = require('typescript');

// get options
const { config } = readConfigFile('tsconfig.json', sys.readFile);
const host = {
  useCaseSensitiveFileNames: false,
  readDirectory: sys.readDirectory,
  fileExists: sys.fileExists,
  readFile: sys.readFile
};
const { options } = parseJsonConfigFileContent(config, host, process.cwd());

// get diagnostics
const rootNames = ['src/index.ts'];
const diagnostics = createProgram({
  rootNames,
  options
}).getSemanticDiagnostics();

console.log(diagnostics);

diagnostics includes the unused vars only if "noUnusedLocals": true, "noUnusedParameters": true, is set, but as far as I understood the suggestions for unused vars are a diagnostic which should be always visible. (I think this was mentioned in #22361.)

Is there an API to get these suggestions? Is this possible with createProgram or do I need createLanguageService (especially when I want to apply the suggestions)? Is there some minimal working example? I couldn't really found a small demonstration of this feature.

Thank you!

---

I tried that, but it doesn't seem to include the unused vars.

// @ts-check
const {
  createLanguageService,
  ScriptSnapshot,
  readConfigFile,
  parseJsonConfigFileContent,
  sys
} = require('typescript');

// get options
const { config } = readConfigFile('tsconfig.json', sys.readFile);
const host = {
  useCaseSensitiveFileNames: false,
  readDirectory: sys.readDirectory,
  fileExists: sys.fileExists,
  readFile: sys.readFile
};
const { options } = parseJsonConfigFileContent(config, host, process.cwd());

// create language server
const filePaths = ['src/index.ts'];
/** @type {import('typescript').LanguageServiceHost} */
const lshost = {
  getCompilationSettings: () => options,
  getScriptFileNames: () => filePaths,
  getScriptVersion: () => '',
  getScriptSnapshot: (name) => ScriptSnapshot.fromString(name),
  getDefaultLibFileName: () => 'lib.d.ts',
  getCurrentDirectory: () => ''
};
const ls = createLanguageService(lshost);
const result = ls.getSuggestionDiagnostics(filePaths[0]);

console.log(result);  // just an empty array

(Source)

[ajafff]

@donaldpipowitch there is an internal method Program#getSuggestionDiagnostics that does what you want. Seems not intended for public use, otherwise it would be exposed in the public API.

[donaldpipowitch]

@ajafff Interesting, thank you! Just tried it, but got an Cannot read property 'id' of undefined error 🤔 However it looks like the language service exposes a getSuggestionDiagnostics as well (and it is public ❤️). It doesn't throw this time, but sadly the result was still empty. I updated my example in the comment above you and added some source code. I think this is the correct method, but I guess I did something wrong in the language server options.

[alexeagle]

I started on a program tswarn which I'd like to distribute with TypeScript:
https://gist.github.com/alexeagle/58e079e91b8577eafb59323f95a0d617
It does exactly this: bring up a LanguageService like an editor would, then emit the diagnostics on the command-line, with a strong preference for being incremental based on the VCS edits (so we don't spam you with existing warnings, nor introduce a wall of output when you enable a new check).

[RyanCavanaugh]

A lot has happened since this issue got opened and it'd be great to get a fresh take on what needs to happen, why, and how

[dallonf]

I think the example that comes most easily to mind is to distinguish between unused variable errors and type mismatch - both are great to know about in CI or a production build. But in local development, it helps to have more granularity. Right now it's very difficult to configure a local Webpack server (for example) to distinguish between type errors, which I might want to fail loudly, and unused variables, which are perfectly expected while I'm developing and I wouldn't want much more than a warning about them.

[elektronik2k5]

@RyanCavanaugh, exactly as @dallonf said, all I want as a user is for the compiler to be able to tell me, as well as supporting ecosystem tooling, whether a given source code issue prevents compilation or not.

Example:

• In production or build time, an unused variable CAN be considered a compilation error. We have this! 🎉 😃
• In development however, an unused variable should not be a compilation ERROR, but a warning. We don't have this. 🤕

Basically what we want is, in addition to the current boolean compilation flags "on and fail" or "off and ignore", a new one: "on and warn" setting - globally (where applicable), as well as per (applicable) compiler option.

[kylecordes]

Traditionally compilers of many types (especially C and C++) had a "treat warnings as errors" flag, enabling more or less exactly what @elektronik2k5 described. This flag was often something like -W. I wonder if something similar could be added to tsc (and its programmatic APIs). But it looks like this was discussed and declined in: #19126

Incidentally a similar feature was discussed and declined in tslint - having found this feature spectacularly useful for many years, I admit being somewhat mystified why it is not considered valuable today.

[donaldpipowitch]

it'd be great to get a fresh take on what needs to happen, why, and how

For me personally I'd like to see a small standalone self-contained example/script how I can actually use the suggestions feature with applying the suggested fixes. I personally don't know how to use it programmatically correctly currently.

Reading the other comments it seems to me that these suggestions maybe should need a general introduction to the community in the first place? With a given outline if and how the suggestions will supersede the current compiler options like noUnusedParameters. (I could imagine to deprecate compiler options like noUnusedParameters and to build the "warn in development"/"error in production"-functionality many seem to desire on top of the suggestions - with the added benefit of being fixable.)

Besides these steps above I personally outlined some ideas/wiches regarding linter functionality once in this ticket #15589 🙃

[donaldpipowitch]

I recently tried to use diagnostics programmatically again. (And failed again...) In #13408 (comment) I tried to use the Node API, but my diagnostics were empty and so far I haven't found a solution.
Now I tried to use the tsserver directly, but I only got "No Project." errors and I don't know how to fix them. (My attempt.)

Is there some guideline or introduction how diagnostics and suggestions can be used programmatically by now? Some of the requests in this thread can probably be solved in community land, but at least for my own use cases I can't find the right resources to see how I can get started.

[uniqueiniquity]

Hi @donaldpipowitch, I took at your original example (autofix-unused.js). Your suspicion about the language server options was correct; your implementation of getScriptSnapshot is causing the language service to read the contents of src/index.ts as src/index.ts. If you change it to the following or similar:

getScriptSnapshot: (name) => ScriptSnapshot.fromString(sys.readFile(name) || ""),

then you should be able to get the two unused local diagnostics as you expected.

As far as providing a guideline or introduction for doing this sort of work, given that you were most of the way there, I think it would be best for us to look into better documenting the API, especially regarding how to put together a mock language service like you've done.

[donaldpipowitch]

@uniqueiniquity Wow, you don't know how happy I am for your little hint. This fixes my problems and I now get suggestion diagnostics. Could you give me one more hint how I can actually "apply" the suggested changes? Is this even built into TypeScript itself or a task for a client (like VS Code)?

E.g. would something like this be correct or is there some higher API I could use?

suggestionDiagnostics.forEach(
  ({ file: { fileName, pos: start, end }, code }) => {
    const codeFixActions = ls.getCodeFixesAtPosition(
      fileName,
      start,
      end,
      [code],
      {},
      {}
    );
    codeFixActions.forEach(({ changes }) => {
      // loop over `changes` to manually change `text` of `file`?
    });
  }
);

Update

The client should take care of applying the changes. (Source)

[cagross]

I'll just leave my two cents on the matter.

TL;DR I would love to see this issue resolved. I want to use VSCode's type checking feature to improve as a programmer. But the issues described here bring so many drawbacks, that I cannot.

I'm new to TypeScript, and VSCode's native type checking feature seemed like a great way to improve myself as a programmer. But the fact that it posts all infractions as red errors in the VSCode Problems tab is a prohibitive issue. I'm working on a team, on a large legacy code base. When I enable VSCode's type chcecking feature, every file I open displays tens (or hundreds) of pre-exsiting type checking violations, as errors in the problem tab. This severely clutters that tab with items that are really just warnings. I rely on that tab to report critical errors in my code. So if it is always cluttered with hundreds of warnings, that utility is gone. And for me right now, that drawback far outweights any benefit gleaned by VSCode's type checking feature. So I must keep it disabled.

[tombohub]

Sometimes when I import function from other file and not sure complete name it immediately show it's an error. But it could be 2 reasons: either function is wrong name or the variable is not used. I have to hover over the line to see what error it is. It would be nice if noUnusedLocal is set to warning only.