-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add noUnnecessaryTypeAssertions compiler option, quickfix #51527
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19882,6 +19882,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
} | ||
} | ||
if (sourceFlags & TypeFlags.Conditional) { | ||
if ((source as ConditionalType).root === (target as ConditionalType).root) { | ||
// Two instantiations of the same conditional type, just check instantiated outer type parameter equality | ||
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); | ||
} | ||
if ((source as ConditionalType).root.isDistributive === (target as ConditionalType).root.isDistributive) { | ||
if (result = isRelatedTo((source as ConditionalType).checkType, (target as ConditionalType).checkType, RecursionFlags.Both, /*reportErrors*/ false)) { | ||
if (result &= isRelatedTo((source as ConditionalType).extendsType, (target as ConditionalType).extendsType, RecursionFlags.Both, /*reportErrors*/ false)) { | ||
|
@@ -26071,6 +26078,24 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
node.kind === SyntaxKind.PropertyDeclaration)!; | ||
} | ||
|
||
function getControlFlowContainerForIdentifier(node: Identifier, declaration: Declaration, symbol: Symbol, typeFromSymbol: Type) { | ||
// The declaration container is the innermost function that encloses the declaration of the variable | ||
// or parameter. The flow container is the innermost function starting with which we analyze the control | ||
// flow graph to determine the control flow based type. | ||
const isParameter = getRootDeclaration(declaration).kind === SyntaxKind.Parameter; | ||
const declarationContainer = getControlFlowContainer(declaration); | ||
let flowContainer = getControlFlowContainer(node); | ||
// When the control flow originates in a function expression or arrow function and we are referencing | ||
// a const variable or parameter from an outer function, we extend the origin of the control flow | ||
// analysis to include the immediately enclosing function. | ||
while (flowContainer !== declarationContainer && (flowContainer.kind === SyntaxKind.FunctionExpression || | ||
flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethodOrAccessor(flowContainer)) && | ||
(isConstVariable(symbol) && typeFromSymbol !== autoArrayType || isParameter && !isSymbolAssigned(symbol))) { | ||
flowContainer = getControlFlowContainer(flowContainer); | ||
} | ||
return flowContainer; | ||
} | ||
|
||
// Check if a parameter or catch variable is assigned anywhere | ||
function isSymbolAssigned(symbol: Symbol) { | ||
if (!symbol.valueDeclaration) { | ||
|
@@ -26432,24 +26457,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
} | ||
|
||
type = getNarrowableTypeForReference(type, node, checkMode); | ||
|
||
// The declaration container is the innermost function that encloses the declaration of the variable | ||
// or parameter. The flow container is the innermost function starting with which we analyze the control | ||
// flow graph to determine the control flow based type. | ||
const isParameter = getRootDeclaration(declaration).kind === SyntaxKind.Parameter; | ||
const declarationContainer = getControlFlowContainer(declaration); | ||
let flowContainer = getControlFlowContainer(node); | ||
const isOuterVariable = flowContainer !== declarationContainer; | ||
const isSpreadDestructuringAssignmentTarget = node.parent && node.parent.parent && isSpreadAssignment(node.parent) && isDestructuringAssignmentTarget(node.parent.parent); | ||
const flowContainer = getControlFlowContainerForIdentifier(node, declaration, localOrExportSymbol, type); | ||
const isModuleExports = symbol.flags & SymbolFlags.ModuleExports; | ||
// When the control flow originates in a function expression or arrow function and we are referencing | ||
// a const variable or parameter from an outer function, we extend the origin of the control flow | ||
// analysis to include the immediately enclosing function. | ||
while (flowContainer !== declarationContainer && (flowContainer.kind === SyntaxKind.FunctionExpression || | ||
flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethodOrAccessor(flowContainer)) && | ||
(isConstVariable(localOrExportSymbol) && type !== autoArrayType || isParameter && !isSymbolAssigned(localOrExportSymbol))) { | ||
flowContainer = getControlFlowContainer(flowContainer); | ||
} | ||
const isOuterVariable = getControlFlowContainer(node) !== declarationContainer; | ||
const isSpreadDestructuringAssignmentTarget = node.parent && node.parent.parent && isSpreadAssignment(node.parent) && isDestructuringAssignmentTarget(node.parent.parent); | ||
// We only look for uninitialized variables in strict null checking mode, and only when we can analyze | ||
// the entire control flow graph from the variable's declaration (i.e. when the flow container and | ||
// declaration container are the same). | ||
|
@@ -32762,23 +32775,29 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
} | ||
|
||
function checkAssertionWorker(errNode: Node, type: TypeNode, expression: UnaryExpression | Expression, checkMode?: CheckMode) { | ||
let exprType = checkExpression(expression, checkMode); | ||
const exprType = checkExpression(expression, checkMode); | ||
if (isConstTypeReference(type)) { | ||
if (!isValidConstAssertionArgument(expression)) { | ||
error(expression, Diagnostics.A_const_assertions_can_only_be_applied_to_references_to_enum_members_or_string_number_boolean_array_or_object_literals); | ||
} | ||
return getRegularTypeOfLiteralType(exprType); | ||
} | ||
checkSourceElement(type); | ||
exprType = getRegularTypeOfObjectLiteral(getBaseTypeOfLiteralType(exprType)); | ||
const targetType = getTypeFromTypeNode(type); | ||
if (!isErrorType(targetType)) { | ||
addLazyDiagnostic(() => { | ||
const widenedType = getWidenedType(exprType); | ||
const regularType = getRegularTypeOfObjectLiteral(getBaseTypeOfLiteralType(exprType)); | ||
const widenedType = getWidenedType(regularType); | ||
if (!isTypeComparableTo(targetType, widenedType)) { | ||
checkTypeComparableTo(exprType, targetType, errNode, | ||
checkTypeComparableTo(regularType, targetType, errNode, | ||
Diagnostics.Conversion_of_type_0_to_type_1_may_be_a_mistake_because_neither_type_sufficiently_overlaps_with_the_other_If_this_was_intentional_convert_the_expression_to_unknown_first); | ||
} | ||
// Assertions, as a side effect, disable widening - some casts may rely on this, so | ||
weswigham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 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.noUnnecessaryTypeAssertions, errNode, Diagnostics.Type_assertion_has_no_effect_on_the_type_of_this_expression); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is the core of the addition, and pretty much everything else in |
||
} | ||
}); | ||
} | ||
return targetType; | ||
|
@@ -32791,8 +32810,38 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
} | ||
|
||
function checkNonNullAssertion(node: NonNullExpression) { | ||
return node.flags & NodeFlags.OptionalChain ? checkNonNullChain(node as NonNullChain) : | ||
getNonNullableType(checkExpression(node.expression)); | ||
const exprType = checkExpression(node.expression); | ||
const resultType = node.flags & NodeFlags.OptionalChain ? checkNonNullChain(node as NonNullChain) : getNonNullableType(exprType); | ||
if (!isErrorType(resultType)) { | ||
addLazyDiagnostic(() => { | ||
weswigham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (isTypeIdenticalTo(exprType, resultType)) { | ||
weswigham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// A non-null assertion on an identifier may also suppress a used-before definition, so may still be useful even if the type is unchanged by it | ||
// Identifiers on the left-hand-side of an assignment expression, ofc, can't do this, since they're for the assigned type itself. | ||
if (isIdentifier(node.expression) && !containsUndefinedType(exprType) && !(isAssignmentExpression(node.parent) && node.parent.left === node)) { | ||
const symbol = getSymbolAtLocation(node.expression); | ||
const declaration = symbol?.valueDeclaration; | ||
if (declaration) { | ||
const isParameter = getRootDeclaration(declaration).kind === SyntaxKind.Parameter; | ||
if (!isParameter) { | ||
const flowContainer = getControlFlowContainerForIdentifier(node.expression, declaration, symbol, exprType); | ||
// 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 | ||
Comment on lines
+32827
to
+32828
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is feels weird. Shouldn't that be a separate concern from CFA? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But not unprecedented! We do this for speculative/constructed control flow queries elsewhere already. See usages of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🤷It's not, though. Last couple lines of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
const fakeReference = factory.cloneNode(node.expression); | ||
setParent(fakeReference, node.parent); | ||
setTextRange(fakeReference, node.expression); | ||
fakeReference.flowNode = node.expression.flowNode; | ||
const flowType = getFlowTypeOfReference(fakeReference, exprType, undefinedType, flowContainer); | ||
if (containsUndefinedType(flowType)) { | ||
return; // Cast is required to suppress a use before assignment control flow error | ||
} | ||
} | ||
} | ||
} | ||
errorOrSuggestion(!!compilerOptions.noUnnecessaryTypeAssertions, node, Diagnostics.Type_assertion_has_no_effect_on_the_type_of_this_expression); | ||
} | ||
}); | ||
} | ||
return resultType; | ||
} | ||
|
||
function checkExpressionWithTypeArguments(node: ExpressionWithTypeArguments | TypeQueryNode) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -800,6 +800,15 @@ const commandOptionsWithoutBuild: CommandLineOption[] = [ | |
description: Diagnostics.Raise_an_error_when_a_function_parameter_isn_t_read, | ||
defaultValueDescription: false, | ||
}, | ||
{ | ||
name: "noUnnecessaryTypeAssertions", | ||
type: "boolean", | ||
affectsSemanticDiagnostics: true, | ||
affectsBuildInfo: true, | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oooh boy. The last I remember hearing of "linting" style features in TypeScript around As much as I love making this a feature, we've already come into conflict with the 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 Sans the compiler option to make this into an error, this check & quick fix is almost a carbon copy of our unneeded There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 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.
+1. https://github.com/Quramy/typescript-eslint-language-service is the closest I've seen used in the wild.
Separate from the performance concerns, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||
{ | ||
name: "exactOptionalPropertyTypes", | ||
type: "boolean", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { | ||
Diagnostics, findAncestor, isAssertionExpression, isInJSDoc, Node, | ||
isParenthesizedExpression, ParenthesizedExpression, SourceFile, textChanges, TextSpan, tryCast, HasJSDoc, first, last, needsParentheses, isNonNullExpression, AssertionExpression, NonNullExpression, | ||
} from "../_namespaces/ts"; | ||
import { codeFixAll, createCodeFixAction, findAncestorMatchingSpan, registerCodeFix } from "../_namespaces/ts.codefix"; | ||
|
||
const fixId = "removeUnnecessaryTypeAssertion"; | ||
const errorCodes = [ | ||
Diagnostics.Type_assertion_has_no_effect_on_the_type_of_this_expression.code, | ||
]; | ||
|
||
registerCodeFix({ | ||
errorCodes, | ||
getCodeActions: function getCodeActionsToRemoveUnnecessaryTypeAssertion(context) { | ||
const changes = textChanges.ChangeTracker.with(context, t => makeChange(t, context.sourceFile, context.span)); | ||
if (changes.length > 0) { | ||
return [createCodeFixAction(fixId, changes, Diagnostics.Remove_unnecessary_type_assertion, fixId, Diagnostics.Remove_all_unnecessary_type_assertions)]; | ||
} | ||
}, | ||
fixIds: [fixId], | ||
getAllCodeActions: context => { | ||
return codeFixAll(context, errorCodes, (changes, diag) => makeChange(changes, diag.file, diag)); | ||
}, | ||
}); | ||
|
||
function makeChange(changeTracker: textChanges.ChangeTracker, sourceFile: SourceFile, span: TextSpan) { | ||
let node: Node | undefined = findAncestorMatchingSpan(sourceFile, span); | ||
if (node && isInJSDoc(node)) { | ||
node = findAncestor(node, isParenthesizedExpression); | ||
if (node) { | ||
changeTracker.deleteNodeRange(sourceFile, first((node as HasJSDoc).jsDoc!), last((node as HasJSDoc).jsDoc!)); | ||
if (!needsParentheses((node as ParenthesizedExpression).expression)) { | ||
changeTracker.replaceNode(sourceFile, node, (node as ParenthesizedExpression).expression); | ||
} | ||
} | ||
return; | ||
} | ||
const castExpr = tryCast(node, (n): n is AssertionExpression | NonNullExpression => isAssertionExpression(n) || isNonNullExpression(n)); | ||
if (!castExpr) { | ||
return; | ||
} | ||
|
||
changeTracker.replaceNode(sourceFile, castExpr, castExpr.expression); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.