Skip to content
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

check index access for fixed length tuple #26292

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Aug 8, 2018

Fixes #5203

@Kingwl Kingwl force-pushed the tupleIndexAccessCheck branch from 7c9d1d4 to 3421531 Compare August 9, 2018 09:49
@@ -18293,6 +18293,13 @@ namespace ts {
error(indexExpression, Diagnostics.A_const_enum_member_can_only_be_accessed_using_a_string_literal);
return errorType;
}
if (isTupleType(objectType) && !objectType.target.hasRestElement && isNumericLiteral(indexExpression)) {
const index = +indexExpression.text;
const maximumIndex = length(objectType.target.typeParameters);
Copy link
Contributor

@IllusionMH IllusionMH Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed after message change.

Looks like there should be length(...) - 1 to get last available index (or add - 1 to error param). Otherwise errors looks off by 1 and confusing.

P.S. Thank you for PR. Glad to see that this will be in TS soon. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, my fault

let x = [] as [];
let y = x[0];
~
!!! error TS2733: Indexed access '0' is out of range of tuple, the maximum of index is '0'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like empty tuples will require separate handling & error message to avoid the maximum of index is '-1'.

@@ -2437,6 +2437,10 @@
"category": "Error",
"code": 2732
},
"Indexed access '{0}' is out of range of tuple, the maximum of index is '{1}'.": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed phrasing

Index '{0}' is out-of-bounds in tuple of length {1}

cc @DanielRosenwasser

@Kingwl Kingwl force-pushed the tupleIndexAccessCheck branch from 3421531 to e90f645 Compare August 10, 2018 01:34
@@ -18293,6 +18293,13 @@ namespace ts {
error(indexExpression, Diagnostics.A_const_enum_member_can_only_be_accessed_using_a_string_literal);
return errorType;
}
if (isTupleType(objectType) && !objectType.target.hasRestElement && isNumericLiteral(indexExpression)) {
const index = +indexExpression.text;
const maximumIndex = length(objectType.target.typeParameters) - 1;
Copy link
Contributor

@IllusionMH IllusionMH Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I had to remove my comments as soon as @RyanCavanaugh proposed to just change error message text (which I think better solution - it handled cases with empty tuples well without special treatment).
With new message - changes that I proposed earlier only add off by one error in mesage 😞 and should be reverted.

@Kingwl Kingwl force-pushed the tupleIndexAccessCheck branch from e90f645 to 6432bd9 Compare August 10, 2018 02:50
@RyanCavanaugh RyanCavanaugh merged commit 6465e9d into microsoft:master Sep 5, 2018
@Kingwl Kingwl deleted the tupleIndexAccessCheck branch September 5, 2018 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants