search

microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
98.27k stars 12.21k forks source link

Lint to ensure CompilerOptions and options in commandLineParser are synchronized #58312

Open weswigham opened 2 weeks ago

weswigham commented 2 weeks ago

A number of our @internal options were missing from our option definitions in commandLineParser.ts, which in turn causes us to elide them from telemetry, even though we may forcibly set them in the language service.

This PR adds a lint rule that compares the CompilerOptions interface and the option definitions in commandLineParser.ts and ensures their state is synchronized. It also adds an internal field to command line option definitions whose only purpose is tracking which options are @internal annotated. This is currently unused but should be a good runtime indicator to anyone looking over our option definitions which ones aren't meant to be used publicly. This field is also synchronized by the lint.

weswigham commented 2 weeks ago

This PR no longer has any functional changes, and instead just has a lint rule to check that commandLineParser.ts and the CompilerOptions interface in types.ts are synchronized. We could do this at compile-time with type arithmetic (though not the @internal sync bit), but it'd require as consting most of the option definitions and collections in commandLineParser.ts, which seems a little heavyweight when we can't easily outright derive CompilerOptions from the option definitions (and so would still need quite a bit of duplication) because there's a collection of 3 calculated fields on CompilerOptions we use to smuggle path/origin information for the options around (which this lint rule exempts from the sync check).

Running it exposed a couple more internal compiler options we didn't have option definitions for that I didn't already know about.

jakebailey commented 2 weeks ago

It feels a little weird to enforce this via a lint rule, but I guess if it's too annoying to do statically...

sheetalkamat commented 2 weeks ago

we have testcase https://github.com/microsoft/TypeScript/blob/cd566bad95124aded20f4cd35e95810077500e40/src/testRunner/unittests/config/commandLineParsing.ts#L252 which checks if all affectsSemanticDiagnostics are marked as affectsBuildInfo as well. May be some test case like that would be better than lint rule