Typescript: 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 https://github.com/Microsoft/TypeScript/issues/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
https://github.com/palantir/tslint/pull/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 [e-mail]
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
https://github.com/palantir/tslint/issues/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
https://github.com/Microsoft/TypeScript/issues/13408#issuecomment-336952524,
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.

[alexeagle]

The TypeScript team doesn't want to take on the governance of such a large
codebase and set of use cases, given what's on their plate. I think that's
why Mohamed wishes they hadn't started doing linting without careful
thought first.

As for having two different tools, I think this is a benefit, because you
want type-checking before you run the program (saves you time vs. finding
the problem at runtime), but linting nits are not important until your code
is ready for review by others (wastes your time to do it for code you may
not use).

On Mon, Oct 16, 2017 at 2:20 PM Jeremy Morton [e-mail]
wrote:

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 already transpiling anyway, one of the services is
could also offer is linting.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/Microsoft/TypeScript/issues/13408#issuecomment-336981152,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC5I_87PEGXFwMgk7Ozj4h9GshYerXhks5ss55RgaJpZM4LgcLC
.

[alexeagle]

Also a note: third-parties might want to add checks that should be run at
the same time as type-checking and which are reported as errors. We do this
in the Bazel typescript compiler (used internally at google)
https://github.com/bazelbuild/rules_typescript/blob/master/internal/tsc_wrapped/strict_deps.ts
That is a plugin for tsc that breaks the build with an error if you use a
symbol from a dependency you didn't declare (prevents depending on
transitive deps)

On Mon, Oct 16, 2017 at 2:24 PM Alex Eagle [e-mail] wrote:

The TypeScript team doesn't want to take on the governance of such a large
codebase and set of use cases, given what's on their plate. I think that's
why Mohamed wishes they hadn't started doing linting without careful
thought first.

As for having two different tools, I think this is a benefit, because you
want type-checking before you run the program (saves you time vs. finding
the problem at runtime), but linting nits are not important until your code
is ready for review by others (wastes your time to do it for code you may
not use).

On Mon, Oct 16, 2017 at 2:20 PM Jeremy Morton [e-mail]
wrote:

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 already transpiling anyway, one of the
services is could also offer is linting.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/Microsoft/TypeScript/issues/13408#issuecomment-336981152,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC5I_87PEGXFwMgk7Ozj4h9GshYerXhks5ss55RgaJpZM4LgcLC
.

[jez9999]

The TypeScript team doesn't want to take on the governance of such a large codebase and set of use cases, given what's on their plate. I think that's why Mohamed wishes they hadn't started doing linting without careful thought first.

In which case the tslint team need to be told very clearly not to remove "dupes" that TS already does because it can provide a warning level error which TypeScript will not do.

[ajafff]

In which case the tslint team need to be told very clearly not to remove "dupes" that TS already does

We just try to minimize the maintenance cost that comes with very complex rules. no-unused-variable was almost removed because it was complex, had many bugs and there was a better alternative available in the compiler.

It's open source software after all. If TSLint decides to drop some rules, you can simply use the code as a custom rule (with proper copyright attribution if you want to publish it).

because it can provide a warning level error which TypeScript will not do.

That's no longer true since the next version of typescript will use a warning severity for lint-like errors.

[jez9999]

So we're officially embracing a mish-mash where tslint does some linting and TS does some. Just so we're clear.

[donaldpipowitch]

For that matter, why not move all linting functionality into TypeScript as configurable warnings/errors/off?

I asked for a similar thing here: https://github.com/Microsoft/TypeScript/issues/15589.

As far as I understand @mhegazy this currently just not aligns with the goals given to the TypeScript project and can't be managed given the current resources. If we really wanted to have linting as a core feature (and to become officially backed by MicroSoft in this regard) there would probably be the need to form a new team co-existing to the TypeScript team (similar to the VA Code team).

I personally would love to see that and ideally help to support that goal, but this is nothing we can decide.

My understanding is this:

While the TypeScript team "fixes" the current established linter-like checks, _if it sufficient_ by introducing "smaller" features like warning severity, linting is generally out of scope and is in the hand of 3rd party tools like TSLint.

Is this correct, @mhegazy?

[mhegazy]

@alexeagle has a write up on this thoughts about this issue in https://gist.github.com/alexeagle/349887ddb277aa5833643fc8c0b5465b

[egamma]

A good write up from @alexeagle.

I do agree on the proposal to move lint like diagnostics to tslint. However, as a tslint integrator, I propose to go further and I also suggest that the tslint project provides a first class tslint TypeScript Language Service plugin.

Why?

• the tslint TypeScript Language Service plugin is tool independent and should be provided as a first class additional option for consuming tslint by the tslint project.
• Some popular lint rules like no-unused-variable require type information and supporting rules that require type information is best done in a language service plugin, see this issue/comment.

How?

• The tslint project could start with the work that I've done with @angelozerr on the tslint-language-service.
• The TypeScript project needs to fill gaps to enable that tslint language service plugin is on par with existing editor integrations like vscode-tslint.

Since these proposals will take longer I still suggest to go with the proposal from above https://github.com/Microsoft/TypeScript/issues/13408#issuecomment-335256365 as a tactical step. It will improve the usage of tslint when tslint rules are used together with TypeScript provided lint checks.

[angelozerr]

The tslint project could start with the work that I've done with @angelozerr on the tslint-language-service.

It was my wish when I have created tslint-language-service. Thanks @egamma to speak about this idea! It should be fantastic if tslint could host language-service for tslint like @angular hosts language-service for angular. With just a simple install

npm install tslint

or

npm install tslint/language-service

any editor which supports tsserver could support tslint validation.

[ffxsam]

As a newcomer to TypeScript, I find it rather confusing that there are two places to define similar rules:

tsconfig.json: noUnusedLocals
tslint.json: no-unused-variables

In fact, in VS Code, the latter will not even check properly unless the former is defined. But if I set noUnusedLocals to true, there are no severity levels (everything is an error) and it causes my app to fail to load if I have an unused var. This isn't a great developer experience.

[alexeagle]

I think this is fixed or nearly fixed by #19392 - the Language Service now produces "suggestions" which are a lower severity than error.

[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 https://github.com/Microsoft/TypeScript/pull/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! :tada: :smiley:
• In development however, an unused variable should not be a compilation ERROR, but a warning. We don't have this. :face_with_head_bandage:

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: https://github.com/Microsoft/TypeScript/pull/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 https://github.com/Microsoft/TypeScript/issues/15589 🙃

[donaldpipowitch]

I recently tried to use diagnostics programmatically again. (And failed again...) In https://github.com/Microsoft/TypeScript/issues/13408#issuecomment-412438318 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)