Add optional type information to rules

[ScottSWu]

We would like to add type information to nodes and give rules access to the type checker, thus resolving #680. To maintain compatibility with current rules, a tentative design is to accept a ts.Program object in the Linter, and pass that to rules that implement an "applyWithProgram" function. A PR is in the works for this feature.

From the cli, constructing the program would require tsconfig for compiler options. Would it be possible to add an argument in the TSLint cli for the config location and/or whether or not to enable type checking?

[adidahiya]

Sure, adding a CLI option to point to a tsconfig.json file to pick up compiler options makes sense. See #858.

A lot of people are very interested in #680. Would you mind writing up a slightly longer proposal of how you plan to approach this? How will you construct a ts.Program to pass to the Linter?

Re: backcompat -- a new Rule method makes sense, but its API probably needs to be more clear than simply "applyWithProgram" (because a walker is also involved). One alternative might be to make a new base class that extends Lint.RuleWalker and takes a ts.Program in its constructor -- something like Lint.ProgramAwareRuleWalker.

[ScottSWu]

Sure, creating the ts.Program object would look something like:

• Obtain (or find) path to tsconfig.json
• Pass the file contents and directory path to ts.parseJsonConfigFileContent
• Call ts.createProgram with the parsed fileNames and options, and a standard compiler host (for test.ts, the compiler host needs to be modified a bit because TypeScript will not compile .lint files)
• Run the type checker by calling ts.getPreEmitDiagnostics

Some other changes tried in this fork:

• Extend AbstractRule to TypedRule, which has the applyWithProgram method
• Added the ts.Program and ts.TypeChecker objects to RuleWalker (now that you mention it, extending RuleWalker would be better)
• Walkers access the type checker through getTypeChecker()
• Linter receives program as an optional argument
• Linter.lint calls applyWithProgram for TypedRules if a program is available, and apply otherwise

[jkillian]

cc @myitcv and @alexeagle who have been involved in this discussion previously

[alexeagle]

thanks @jkillian - actually @ScottSWu and I are working together on this proposal.
We also should sync up with how Wes proposes this in TS: look at microsoft/TypeScript#9038 if you haven't already.

[alexeagle]

Did @ScottSWu 's update address the design questions about this? We'd like to get a PR going soon to move our work upstream. Would you like a longer design/proposal?

[jkillian]

Sorry for the lack of response @alexeagle and @ScottSWu. I had thought that you were putting this on hold until we saw what became of microsoft/TypeScript#9038. I'll take a more detailed look at your proposal and work so far @ScottSWu either this weekend or early next week

[jkillian]

I like your proposal @ScottSWu, it seems like a smooth way to add the feature that doesn't cause breaking changes. A couple quick thoughts:

• Users running TSLint via its JA API shouldn't have to know how to create a ts.Program and set up everything themselves - will we expose functionality to make this easier if not using the CLI? Maybe this implies that we'd need a new ProgramLinter (naming up for debate) class that could use Linter internally. API users could choose which of the two they wanted to use programatically, whereas the CLI would choose based on if a tsconfig.json was provided or not.
• What happens if a user tries to run a rule that requires a ts.Program but they're running TSLint in such a way that it's not available? Are the apply and applyWithProgram methods ever both going to be defined on a single Rule?

[ScottSWu]

• The TypeScript API makes parsing the tsconfig file and creating a program convenient. Would adding an example of this to the docs be sufficient for people using the API?

const {config, error} = ts.readConfigFile("tsconfig.json", ts.sys.readFile);
const parsed = ts.parseJsonConfigFileContent(config, {readDirectory: ts.sys.readDirectory}, path);
const host = ts.createCompilerHost(parsed.options, true);
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
const fileNames = program.getSourceFiles().map(s => s.fileName).filter(l => l.substr(5) !== ".d.ts");

• At the moment, a TypedRule implements apply as return [], which would effectively skip it if no program is supplied. Should there be some sort of warning if this happens (e.g. "you are trying to use a rule that requires a program")?

[jkillian]

It seems a little bit complex, although an example in the docs would obviously help with that a lot. Would you anticipate people using the exact same code every time essentially? If so, it seems that we could at least optionally abstract things for API users.

[ScottSWu]

It would probably be the same for projects that do not already compile through the typescript API. Should the Linter have some sort of getProgram function that takes the tsconfig path (and project directory) as inputs?

[alexeagle]

Our thinking is that most developer workflows that call tslint also do type-checking. Type-checking takes something like 3x the time of linting, so it would be better not to type-check the program twice. Ideally I think that programs that use the proposed Linter API that accepts a ts.Program would do so using a ts.Program object they've already created and type-checked, not using a utility provided by tslint to create a Program from a path to tsconfig.json (though I agree that some users would prefer such a utility be included).

For example, API users like gulp/grunt plugins would get the ts.Program that is already created (eg. in the gulp-typescript plugin, which would have to expose it somehow) and call the tslint API including that. Editor plugins similarly can get the current ts.Program from the TS language services.

When/if the https://github.com/Microsoft/TypeScript/tree/extensibility-model branch is merged, we'll have an easy time explaining how those lint checks can be added into the tsc workflow. This is what we plan to do in Angular's tsc-wrapped which is the extension model we use for the ngc compiler most Angular devs will be encouraged to use (it includes compilation of the Angular templates in addition to .ts sources). Ultimately if the extensibility model goes well, then tsconfig.json could include

compilerOptions: {
  extensions: {
                        "tslint": ["tslint.json"]
...

[jkillian]

That makes sense, seems like a sound proposal. @ScottSWu I think it's a good idea to have a convenience function included, but the Linter constructor can directly take an optional ts.Program object.

[ScottSWu]

Going back to the command line options, other discussions seem to point towards a -p/--project argument which points to the tsconfig.json file. Should this enable typechecking by default or require a flag to enable it (--type-check)? With typechecking seems to take twice as long to run than without

[jkillian]

Let's enable it by default but have an opt-out flag? It seems that it's a worthwhile feature that most people will end up wanting to use, but we don't want to force users to take on negative performance problems.

Just an idea though, I could be persuaded either way

[adidahiya]

I would prefer type checking to be opt-in at first, at least until the next major version. We should let it bake for a little while before flipping the switch to make it a default. Also there might be some users who want TSLint to only do syntactic checking (maybe as a philosophical thing).

[alexeagle]

Should #680 be closed as well?