Suppress error caused by intermediate types in getTypeOfExpression
#54380
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test this |
| @@ -1258,6 +1258,7 @@ export const enum CheckMode { | |||
| RestBindingElement = 1 << 6, // Checking a type that is going to be used to determine the type of a rest binding element | |||
| // e.g. in `const { a, ...rest } = foo`, when checking the type of `foo` to determine the type of `rest`, | |||
| // we need to preserve generic types instead of substituting them for constraints | |||
| TypeOnly = 1 << 7, // Called from getTypeOfExpression, diagnostics may be omitted | |||
There was a problem hiding this comment.
“Type-only” is an overloaded term that sounds like it refers to type-only imports. Would SkipDiagnostics be more clear?
Also, there are a number of mechanisms like this already in the checker, and it’s confusing which one to use. getSignatureApplicabilityError, compareSignaturesRelated, and checkApplicableSignatureForJsxOpeningLikeElement all take both a CheckMode and a reportErrors parameter. It now seems possible for those two to disagree. And there are surely lots of other functions that take a CheckMode but haven’t been updated to suppress errors for TypeOnly.
It seems like it might be better to add use a reportErrors parameter to checkBinaryLikeExpressionWorker, unless we’re prepared to handle this new CheckMode everywhere CheckModes are passed.
There was a problem hiding this comment.
The new flag indicates that the originating call came from getTypeOfExpression, meaning that diagnostics can optionally be omitted with no ill effects. But unless we're dealing with an expression that could possibly cause new errors when operands are given narrower types than their declared types, there is no requirement that diagnostics must be omitted. It's bit of a mouthful, not sure what a good short name for that concept is.
I don't think there's an issue with disagreement between checkMode and reportErrors since the flag can be safely ignored in practically all scenarios--in fact, beyond the error for non-comparable operands, nothing needs to change as of now. But if we later discover other issues similar to this, we now have a mechanism to address them.
The only way to properly propagate the information is through a CheckMode flag.
There was a problem hiding this comment.
Yeah, I kinda agree with Andrew and from time to time I find myself wishing we had a better way to control when to report errors and when to not to based on whether we're actually checking things, or just getting types. But since we're nowhere close to doing this, I'm ok with the CheckMode trick for now.
There was a problem hiding this comment.
Right, in an ideal world we'd have more separation between the process of resolving types and that of checking the types. But given that both of those processes have a lot of common code paths (i.e. the entire top-down recursive decomposition is pretty much the same), we've chosen to conflate the them. This does occasionally muddle things, but it definitely also saves a lot of code.
|
@typescript-bot test this |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at 3343bae. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at 3343bae. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at 3343bae. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the abridged perf test suite on this PR at 3343bae. You can monitor the build here. Update: The results are in! |
|
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@gabritto Here they are:Comparison Report - main..54380
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @gabritto, the results of running the DT tests are ready. |
Fixes #46475.