Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Apr 24, 2024

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.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 24, 2024
@@ -124,7 +124,9 @@ Info seq [hh:mm:ss:mss] event:
"deferred": 0,
"deferredSize": 0
},
"compilerOptions": {},
"compilerOptions": {
"allowNonTsExtensions": true
Copy link
Member Author

@weswigham weswigham Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LS sets allowNonTsExtensions for any project with js and any inferred project forcibly - this wasn't reflected in telemetry (and all these baselines), since without an options declaration, the serializer couldn't serialize the option.

src/compiler/emitter.ts Outdated Show resolved Hide resolved
@weswigham weswigham force-pushed the defensive-option-definitions branch from f494cc6 to d5b24f8 Compare April 26, 2024 20:25
@weswigham weswigham changed the title Unify how @internal CompilerOptions are treated with respect to buildinfo Lint to ensure CompilerOptions and options in commandLineParser are synchronized Apr 27, 2024
@weswigham
Copy link
Member Author

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.

Comment on lines +1628 to +1631
internal: true,
extraValidation() {
return [Diagnostics.Unknown_compiler_option_0, "allowNonTsExtensions"];
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an option is marked internal, can extraValidation always just issue this wherever that func is called? Or is that too annoying? Seems like this is copy/pasted a bunch, but I guess not for anything outside this block?

@jakebailey
Copy link
Member

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

@sheetalkamat
Copy link
Member

we have testcase

it("should have affectsBuildInfo true for every option with affectsSemanticDiagnostics", () => {
which checks if all affectsSemanticDiagnostics are marked as affectsBuildInfo as well. May be some test case like that would be better than lint rule

@sandersn sandersn added this to Not started in PR Backlog May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Not started
Development

Successfully merging this pull request may close these issues.

None yet

4 participants