Skip to content

Conversation

@eellison
Copy link
Contributor

@eellison eellison commented May 2, 2019

Fix for #16962

This needs fixing because we turn lists into tuples when constantify a module, so indexing into a Tuple of one type with a non-constant integer is quite common.

@pytorchbot pytorchbot added the oncall: jit Add this issue/PR to JIT oncall triage queue label May 2, 2019
@eellison eellison requested review from Krovatkin and driazati May 2, 2019 21:37
@eellison
Copy link
Contributor Author

eellison commented May 2, 2019

@pytorchbot retest this please

@eellison
Copy link
Contributor Author

eellison commented May 2, 2019

@pytorchbot rebase this please

/*allow_conversions*/ true);
if (list == tuple_val) {
throw ErrorReport(loc)
<< "Cannot index into a " << tuple_typ->python_str()
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be more clear, maybe something like ... non-constant index because it has multiple element types

Copy link
Contributor Author

@eellison eellison May 3, 2019

Choose a reason for hiding this comment

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

Agreed that would be a little more clear, but then it wouldn't handle the zero-tuple case, and i think it's still clear as is

@eellison eellison requested a review from driazati May 3, 2019 16:45
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@eellison is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@eellison merged this pull request in 26f5275.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants