-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
fix35982: allow BigIntLiteral to parse as PropertyName for literal object and indices #58608
Conversation
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
@typescript-bot test it |
Hey @iisaduan, the results of running the DT tests are ready. Everything looks the same! |
@iisaduan Here are the results of running the user tests comparing Everything looks good! |
@iisaduan Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@iisaduan Here are the results of running the top 400 repos comparing Everything looks good! |
@typescript-bot test it |
Hey @iisaduan, the results of running the DT tests are ready. Everything looks the same! |
@iisaduan Here are the results of running the user tests comparing Everything looks good! |
@iisaduan Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@iisaduan Here are the results of running the top 400 repos comparing Everything looks good! |
@typescript-bot test it |
Hey @iisaduan, the results of running the DT tests are ready. Everything looks the same! |
@iisaduan Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@iisaduan Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
@typescript-bot user test this |
@iisaduan Here are the results of running the user tests with tsc comparing Everything looks good! |
src/compiler/checker.ts
Outdated
error(indexNode, Diagnostics.Property_0_does_not_exist_on_type_1, "" + (indexType as StringLiteralType | NumberLiteralType).value, typeToString(objectType)); | ||
} | ||
else if (indexType.flags & (TypeFlags.String | TypeFlags.Number)) { | ||
error(indexNode, Diagnostics.Type_0_has_no_matching_index_signature_for_type_1, typeToString(objectType), typeToString(indexType)); | ||
} | ||
else { | ||
error(indexNode, Diagnostics.Type_0_cannot_be_used_as_an_index_type, typeToString(indexType)); | ||
const typeString = typeToString(indexNode.kind === SyntaxKind.BigIntLiteral ? getBaseTypeOfLiteralType(getContextFreeTypeOfExpression(indexNode)) : indexType); |
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.
Does this just end up producing bigint
or do we expect something else to show up here? If something other than bigint
can appear, we should add tests as the tests only show bigint
. If this can only ever produce bigint
, then I would just use the literal string.
Also, unless anyone else has a particular preference, I think Type '1n' cannot be used as an index type
is also acceptable, so leaving this as typeToString(indexType)
might be fine.
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.
I'm not really sure how the latest push resolved this; "bigInt" isn't a thing in TS or JS and I think Ron was suggesting to actually write the value? (See also my comment on the earlier thread with suggested code)
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.
We should either write the value, or bigint
(all lowercase) as it's the underlying base type.
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.
Sorry, I've been looking at the tests, and some of the test cases have been inconsistent with the caps. I'll fix that.
Would you prefer the "property does not exist on type" error? I was using the "bigint literal type cannot be used as an index" error because the original design choice was the bigintliteral type cannot be used as an index, as you should use a string or number instead. "cannot be used as index" seemed more descriptive of what was happening than "property does not exist".
I don't mind using the value in the error message (1n cannot be used as an index type
) if you like that better, and it is what you would get with typeToString(indexType)
. I changed it because bigint
made the above more clear as well.
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.
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.
I guess if you just swap it back to bigint
, that should be sufficient. I see what you mean about the inconsistency; it's not that say 1n
doesn't exist on the type, but that bigint isn't supported at all?
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.
Yes, it's valid syntactically in js to index with bigints, but we've disallowed them when type checking
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.
There are a few things that I didn't catch before that I want addressed, but I'm going to mark this as approved so you're not blocked on merging.
Fixes #35982
This PR allows more valid bigint uses in JS to parse and emit correctly. We will parse (and correctly emit) the following cases, but give the following errors: