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

Fixes for covariance/contravariance + #734 #898

Merged
merged 2 commits into from
Oct 12, 2015
Merged

Conversation

jhance
Copy link
Collaborator

@jhance jhance commented Oct 10, 2015

We weren't actually propagating variance to the TypeVarType correctly.

Fixes this and also #734 which was my end goal when writing this small series.

We weren't actually passing in the variance, instead
we were passing in line as the value and letting line
get defaulted.

As a possible fix for things like this not happening
in the future, invariance really should be an enum,
not an int. Otherwise, we don't get protected by the
type system we are making...
Reject covariant type variables being used as an argument
type. Reject contravariant type variables being used as
a return type.

Fixes python#734.
@jhance
Copy link
Collaborator Author

jhance commented Oct 10, 2015

The implementation for #734 provides test coverage for the first fix so I decided to include these together

@jhance
Copy link
Collaborator Author

jhance commented Oct 10, 2015

I have a followup for this that adds checks for inheritance-related variance issues, but because github's model doesn't allow me to submit a PR for commits based on things that aren't in your repository yet, I'll wait until this is merged...

@o11c
Copy link
Contributor

o11c commented Oct 10, 2015

If you submit a PR with (all the commits in one PR) + (all the commits in another PR), then if the first PR is merged, they'll disappear frrom the second PR. I've done that with several of my PRs, though I have to be careful with rebasing them all over the weeks and months that they aren't merged.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 12, 2015

I generally prefer if you can wait until the first PR is merged if you have a second PR on top of that, as things can get very confusing when sending a pull request on top another one that hasn't been merged, especially if the first one requires significant changes based on the review.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 12, 2015

Thanks for the PR, looks good! As mentioned in #734, things like List[T] are also problematic, but this brings an incremental benefit so I can merge this.

JukkaL added a commit that referenced this pull request Oct 12, 2015
Fixes for covariance/contravariance + #734
@JukkaL JukkaL merged commit 6eb1e92 into python:master Oct 12, 2015
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