Add noUnnecessaryTypeAssertions compiler option, quickfix #51527
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
weswigham
force-pushed
the
no-unnessesary-assertions-quickfix
branch
from
7d58f8a to
1988e50
Compare
| const params = (source as ConditionalType).root.outerTypeParameters || []; | ||
| const sourceTypeArguments = map(params, t => (source as ConditionalType).mapper ? getMappedType(t, (source as ConditionalType).mapper!) : t); | ||
| const targetTypeArguments = map(params, t => (target as ConditionalType).mapper ? getMappedType(t, (target as ConditionalType).mapper!) : t); | ||
| return typeArgumentsRelatedTo(sourceTypeArguments, targetTypeArguments, map(params, () => VarianceFlags.Unmeasurable), /*reportErrors*/ false, IntersectionState.None); |
There was a problem hiding this comment.
So, this is kind of a drive-by speed-up for identity checking for conditional types. You'd think this change is unrelated, but since this change triggers identity comparisons in a new location, it discovered a bug!
Usually, identity checking is really fast, since we either bail quickly, or rapidly identify reference-equal types. However, we have an example in our test suite where we cast an expression which has a generative conditional type to what is essentially the same generative conditional type with an (allowable) type parameter substitution. Without this fix, it spun for... well, longer than I cared to wait. This, then, is a drive-by fix for a pathological case in conditional type identity checking I stumbled on. See tests\cases\compiler\conditionalTypeDoesntSpinForever.ts for the relevant test case. By comparing outer type parameter equality when conditional type roots match, we can shortcut pretty much all recursion, preventing the runaway generation and likely being more performant in the general case.
| // if the expression type was widenable, we shouldn't assume the cast is extraneous. | ||
| // (Generally speaking, such casts are better served with `as const` casts nowadays though!) | ||
| else if (widenedType === exprType && isTypeIdenticalTo(targetType, widenedType)) { | ||
| errorOrSuggestion(!!compilerOptions.noUnnecessaryCasts, errNode, Diagnostics.Type_cast_has_no_effect_on_the_type_of_this_expression); |
There was a problem hiding this comment.
This line is the core of the addition, and pretty much everything else in checker is to handle edge cases the eslint rule authors identified - like using a cast to prevent widening (a common occurrence before as const) or using a non-null assertion to prevent a use-before-assignment error on a non-nullable type'd variable.
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 1988e50. You can monitor the build here. |
|
Weird that the self check fails; the stack trace isn't log enough to show context but I'm wondering if there's a bug in the PR which only fires when compiling TS itself, something that isn't in a test case. |
|
Odd that it doesn't repro locally, but I suspect I know the issue. |
|
@DanielRosenwasser as far as terminology goes, I mostly copied the language #32363 used for our existing |
Having briefly looked at the eslint rule, |
| // We need a fake reference that isn't parented to the nonnull expression, since the non-null will affect control flow's result | ||
| // by suppressing the initial return type at the end of the flow |
There was a problem hiding this comment.
This is feels weird. Shouldn't that be a separate concern from CFA?
There was a problem hiding this comment.
But not unprecedented! We do this for speculative/constructed control flow queries elsewhere already. See usages of getSyntheticElementAccess within the checker. I just couldn't reuse it because these aren't element accesses, they're just straight identifiers (sans assertion).
There was a problem hiding this comment.
Shouldn't that be a separate concern from CFA?
🤷It's not, though. Last couple lines of getFlowTypeOfReference is basically "if we still have the initial type, under which contextual circumstances should we return the declared type instead", and those circumstances are basically what we wanna change here.
There was a problem hiding this comment.
@DanielRosenwasser This comment refers to a specific workaround required in the context of the control flow analysis (CFA) process. The aim here is to simulate a reference that doesn't originate from the nonnull expression to ensure that the control flow's result isn't affected solely by the presence of the non-null assertion. This workaround is reminiscent of similar approaches used in speculative or constructed control flow queries elsewhere in the codebase (like getSyntheticElementAccess within the checker). In this scenario, we're dealing with identifiers lacking assertions, and therefore, the approach isn't directly reusable. The concern addressed here is indeed entwined with the logic in the last few lines of getFlowTypeOfReference. Essentially, it delves into deciding, within certain contextual circumstances, when to return the declared type instead of retaining the initial type under which the analysis began. Hope this clarifies the context.
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at fc93cf3. You can monitor the build here. Update: The results are in! |
weswigham
changed the title
|
@DanielRosenwasser I've tightened up the language across all the diagnostics and option name to only use the term |
|
@weswigham Here they are:
CompilerComparison Report - main..51527
System
Hosts
Scenarios
TSServerComparison Report - main..51527
System
Hosts
Scenarios
StartupComparison Report - main..51527
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I really like this, but I don't know if we decided to add it in the design meeting or not (given it's another knob). |
|
It all functions without the compiler option, so that much I think can be unambiguously helpful, given the quick fix - the option just enables reporting it as an error (and thus be a true replacement for the lint rule). |
I'm not sure why I thought this but I always assumed this was one of the intended use cases for non-null assertions. Pretty sure I've used them once or twice for this exact purpose. Maybe control flow analysis has gotten good enough that this is never necessary in real code anymore, though. |
|
Also cc @bradzacher and @JoshuaKGoldberg for interest. |
| category: Diagnostics.Type_Checking, | ||
| description: Diagnostics.Raise_an_error_when_a_type_assertion_does_not_affect_the_type_of_an_expression, | ||
| defaultValueDescription: false, | ||
| }, |
There was a problem hiding this comment.
Oooh boy. The last I remember hearing of "linting" style features in TypeScript around noUnusedLocals and noUnusedParameters was that the team was strongly considering moving away from them? (this comment should not be treated as a source - I'm asking about anecdotal memories)
As much as I love making this a feature, we've already come into conflict with the noUnused* flags in lint land with https://typescript-eslint.io/rules/no-unused-vars. Users often want more flexibility than TypeScript can offer with just compiler options. See https://eslint.org/docs/latest/rules/no-unused-vars#options for options demanded by enough users to be justified adding to ESLint core, and typescript-eslint/typescript-eslint#5271 for a reference on many of the wacky ways users have asked to be able to configure rules.
Is there more context that can be provided on the TypeScript team's opinion on adding more compiler options for lint-area responsibilities? Perhaps an alternate long-term strategy would be to provide an API we can hook into? Perhaps even a ... type relationship API? 😁
There was a problem hiding this comment.
My impetus for suggesting this was #51456, where I was trying to stop type checking during lint as doing so is half of our lint time, which is not so good for editor responsiveness in the project (for those of us who run ESLint in the editor; I think I may be one of the only ones on the team willing to take the perf hit of running it as an extension on this repo).
We only wanted this one rule, which is something that TS could be doing internally pretty easily.
But, I'm not really the one to be answering for the whole team's opinion here.
There was a problem hiding this comment.
If linting with type info is slow for you guys, perhaps we can work together to figure out how we can improve the performance overall?
Obviously we won't be able to get it to the same speed as non-type-aware lint, but perhaps we can close that gap. This would ofc be a great win for the entire community!
There was a problem hiding this comment.
I'd love to come up with ways to do so, though given the time is roughly the time it takes to compile TS itself, it's hard to know what could be improved besides improving the performance of TS itself. (e.g., upgrade to 5.0 😄)
There was a problem hiding this comment.
I mean, it's basically slow because it's duplicating all the work the compiler already does in the language service to fetch errors in a file for the editor, or at least a large part of that work, because calculating the types at a bunch of locations is most of the work the compiler does. I also don't think the eslint editor extension even uses our language service APIs to keep around a persistent project, so it's also not incremental in its parse behavior as far as the TS API is concerned (even though normal lints may be incremental).
Honestly, I'd still love to see linter plugins that could be integrated straight into the compiler at some point, like I implemented back when I was an intern - being able to guarantee a single walk that's incremental, shared across all rules, and reuses checker data where possible would be big cross-project upside, we just are still skittish about a built-in plug-in model after all this time because we're scared both about untrusted code in the editor (maybe passable now that the editor asks if you trust the code) and about being blamed for the performance penalty poorly written plugins may cause (since perf is such a huge concern).
There was a problem hiding this comment.
Is there more context that can be provided on the TypeScript team's opinion on adding more compiler options for lint-area responsibilities?
What we've said on this, specifically, in our discussions at this point is that while we prefer things like this to be external lint rules, if there's outsized cost or capability lost by them being external (eg, because they require fancy control flow checks to properly implement), we're more OK with them living in the compiler for now. So current internal consensus is mostly that stuff like noFallthroughInSwitch really should have been external, since it's a fast syntactic check, noUnusedX is a bit more middling since it's kinda free for us to track while already checking, and stuff like this that needs control flow based definite assignment analysis and relationship checks to answer right is right up the alley of what our quickfixes should pick up, since we're very well-positioned to answer this question quickly during a normal type check.
Sans the compiler option to make this into an error, this check & quick fix is almost a carbon copy of our unneeded await quickfix, so we've had similar functionally knocking around for awhile.
There was a problem hiding this comment.
eslint editor extension even uses our language service APIs
cc @bradzacher - is this something that ESLint today could support?
Most of typescript-eslint is pluggable and supports receiving TypeScript programs. For example, the parser's options can include a program: https://github.com/typescript-eslint/typescript-eslint/blob/07119945fb0c1d6acb421b1b9ba972c8d9c8942a/packages/types/src/parser-options.ts#L53.
This lack of language service / program reuse is one of our biggest pain points. We added an FAQ about out-of-date type info to our site. eslint/eslint#16557 (comment) has context on the ESLint structural side.
linter plugins that could be integrated straight into the compiler at some point
+1. https://github.com/Quramy/typescript-eslint-language-service is the closest I've seen used in the wild.
outsized cost or capability lost by them being external
Separate from the performance concerns, no-unnecessary-type-assertion is one of the less costly rules in TypeScript-ESLint core. It's only a couple hundred lines of real logic. Searching for accepted no-unnecessary-type-assertion issues shows only 22 open since the beginning of 2019. Only 13 of those are bugs. I'd put forward that the maintenance cost on our side for it is relatively quite small.
There was a problem hiding this comment.
is this something that ESLint today could support?
No, we don't know when we're running in the IDE, so we can't spin up a server.
But we default to using a ts.WatchCompilerHostOfConfigFile (and by extension a ts.BuilderProgram), so we are doing incremental builds using persistent programs.
This is also cached forever and reused whenever a file that is included in the program is linted.
|
My memory is that we discussed this in a design meeting and decided not to take it. But I don't see any links to notes here. @weswigham or @DanielRosenwasser do you remember whether we decided anything about this? My feeling on this is basically the same as @JoshuaKGoldberg 's comment: #51527 (comment) And in fact we've got a PR for exposing isTypeAssignableTo now: #56448 |
weswigham
added
the
Experiment
Fixes comments about this PR and allowing us to remove the
no-unnecessary-type-assertioneslint rule (once we LKG and switch this on), since we can do it much more efficiently within the compiler. Unlike the eslint rule, this isn't configurable, but it is strongly based on ouridentitytype relationship, rather than a bunch of heuristics using type flags and object identity. Using compiler internals also allows fixing some longstanding bugs with the lint rule, like the one that prompted this issue, without using heuristics.