Prefer using enum literal's own base type rather than enum's base type in comparison #52703
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
jakebailey
commented
Feb 9, 2023
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at e808551. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at e808551. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at e808551. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at e808551. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at e808551. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at e808551. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at e808551. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
jakebailey
commented
Feb 9, 2023
src/compiler/checker.ts
Outdated
| @@ -36423,8 +36440,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| case SyntaxKind.LessThanEqualsToken: | |||
| case SyntaxKind.GreaterThanEqualsToken: | |||
| if (checkForDisallowedESSymbolOperand(operator)) { | |||
| leftType = getBaseTypeOfLiteralType(checkNonNullType(leftType, left)); | |||
| rightType = getBaseTypeOfLiteralType(checkNonNullType(rightType, right)); | |||
| leftType = getBaseTypeOfLiteralTypeForComparison(checkNonNullType(leftType, left)); | |||
There was a problem hiding this comment.
I suspect this isn't the only case; the AmpersandAmpersandToken block below also uses the old function. But, changing that produces an... odd result:
>rf6 : 0 | E.b | E.c
>a6 && a6 : 0 | E.b | E.c
Because the result is actually used for the output, whereas this comparison block always gives back bool and so the unions would not be visible.
|
@jakebailey Here are the results of running the user test suite comparing Something interesting changed - please have a look. Detailsaxios-src
|
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here they are:
CompilerComparison Report - main..52703
System
Hosts
Scenarios
TSServerComparison Report - main..52703
System
Hosts
Scenarios
StartupComparison Report - main..52703
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsakveo/ngx-admin
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailscheeriojs/cheerio
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsionic-team/ionic-framework
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsn8n-io/n8n
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsprisma/prisma
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsstatelyai/xstate
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailsvuejs/vue
|
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
ahejlsberg
requested changes
Feb 16, 2023
There was a problem hiding this comment.
I suggest changing the new function to this:
function getBaseTypeOfLiteralTypeForComparison(type: Type): Type {
return type.flags & (TypeFlags.StringLiteral | TypeFlags.TemplateLiteral | TypeFlags.StringMapping) ? stringType :
type.flags & (TypeFlags.NumberLiteral | TypeFlags.Enum) ? numberType :
type.flags & TypeFlags.BigIntLiteral ? bigintType :
type.flags & TypeFlags.BooleanLiteral ? booleanType :
type.flags & TypeFlags.Union ? mapType(type, getBaseTypeOfLiteralTypeForComparison) :
type;
}A few things to note about this:
- It widens
TypeFlags.Enum(a computed enum member) to typenumber. - It consistently widens all literal types to
number,string,bigint, orboolean. - It doesn't do caching. There's no need to, this function isn't really time critical.
It looks like you don't have tests for computed enum members. They're of kind TypeFlags.Enum which lacks handling in your version of the function.
|
My suggested change causes some baseline changes because it consistently widens enum members to |
|
BTW, one subtlety to keep in mind is that |
|
Thanks for the feedback! I've taken your suggestion and added some computed tests, along with an update to the comment to explain the new behavior. |
ahejlsberg
approved these changes
Feb 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #52701
When comparing against an enum literal, if we know that literal's underlying type, compare against that rather than falling back to the entire enum's base type. This avoids cases where adding a string to an all-number enum causes the existing enum values to become incomparable (which feels like it's against the point of #50528, to make those mixed-type enums no longer behave oddly).