Skip to content

Avoid type-var collisions in attrs and dataclasses - #5552

Merged
ilevkivskyi merged 2 commits into
python:masterfrom
euresti:type_var_issue
Sep 3, 2018
Merged

ilevkivskyi merged 2 commits into
python:masterfrom
euresti:type_var_issue

Conversation

@euresti

@euresti euresti commented Sep 1, 2018 •

Copy link
Copy Markdown
Contributor

The issue was caused because we were using 1 for the ids of the generated types.
Since by convention TypeVars for methods should have negative numbers and we're creating these methods we're guaranteed to have -1 free. So use that instead.

Fixes #5542

@euresti euresti changed the title Avoid type-var collisions by using -1 instead of 1 for their ids. Avoid type-var collisions in attrs and dataclasses Sep 1, 2018
@ilevkivskyi

Copy link
Copy Markdown
Member

I will close and re-open to trigger CI builds (should be fixed now).

@ilevkivskyi ilevkivskyi closed this Sep 2, 2018
@ilevkivskyi ilevkivskyi reopened this Sep 2, 2018

@ilevkivskyi ilevkivskyi left a comment

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.

Thanks for the fix!

I have only one comment: typically we add original repros from the issue to the tests (sometimes with few tweaks) to avoid regressions.

@ilevkivskyi

Copy link
Copy Markdown
Member

(Also it looks like you still need to update revealed type in one more test.)

By convention TypeVars for methods should have negative numbers.  Since we're creating these methods we're guaranteed
to have -1 free.
Fixes python#5542
@euresti

euresti commented Sep 2, 2018

Copy link
Copy Markdown
Contributor Author

Ok. New test added and tests pass!

@ilevkivskyi ilevkivskyi left a comment

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.

Looks good now, thanks!

@ilevkivskyi
ilevkivskyi merged commit 2c9d11e into python:master Sep 3, 2018
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.

2 participants