Skip to content

check index access for fixed length tuple#26292

Merged
RyanCavanaugh merged 1 commit into
microsoft:masterfrom
Kingwl:tupleIndexAccessCheck
Sep 5, 2018
Merged

check index access for fixed length tuple#26292
RyanCavanaugh merged 1 commit into
microsoft:masterfrom
Kingwl:tupleIndexAccessCheck

Conversation

@Kingwl
Copy link
Copy Markdown
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
Comment thread src/compiler/checker.ts
}
if (isTupleType(objectType) && !objectType.target.hasRestElement && isNumericLiteral(indexExpression)) {
const index = +indexExpression.text;
const maximumIndex = length(objectType.target.typeParameters);
Copy link
Copy Markdown
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
Copy Markdown
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'. No newline at end of file
Copy link
Copy Markdown
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'.

Comment thread src/compiler/diagnosticMessages.json Outdated
"category": "Error",
"code": 2732
},
"Indexed access '{0}' is out of range of tuple, the maximum of index is '{1}'.": {
Copy link
Copy Markdown
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
Comment thread src/compiler/checker.ts Outdated
}
if (isTupleType(objectType) && !objectType.target.hasRestElement && isNumericLiteral(indexExpression)) {
const index = +indexExpression.text;
const maximumIndex = length(objectType.target.typeParameters) - 1;
Copy link
Copy Markdown
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
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants