Skip to content

Make contravariant inferences only from pure contravariant positions #27357

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

Merged
merged 4 commits into from
Sep 26, 2018

Conversation

ahejlsberg
Copy link
Member

Fixes #27337.

@ahejlsberg ahejlsberg changed the title Contravariant inferences only from pure contravariant positions Make contravariant inferences only from pure contravariant positions Sep 26, 2018
@@ -1,3 +1,6 @@



Copy link
Contributor

Choose a reason for hiding this comment

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

I think that these newlines are strange.

const saveBivariant = bivariant;
const kind = target.declaration ? target.declaration.kind : SyntaxKind.Unknown;
// Once we descend into a bivariant signature we remain bivariant for all nested inferences
bivariant = bivariant || kind === SyntaxKind.MethodDeclaration || kind === SyntaxKind.MethodSignature || kind === SyntaxKind.Constructor;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this only need to be set on the specifically contravariant comparison within forEachMatchingParameterType? Also, could we not use a single variance variable for both bivariant and contravariant context tracking (seeing as we have a variance enum already)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The contravariant flag continues to track whether the position is co- or contra-variant, regardless of whether we've descended into a bivariant position. Doing it this way, using two separate boolean variables, makes the logic that flips the flag easy. I looked at keeping it in a single variable but didn't like the complexity that came with it.

@ahejlsberg ahejlsberg merged commit d7219b2 into master Sep 26, 2018
@ahejlsberg ahejlsberg deleted the fixBivariantInferences branch September 26, 2018 21:04
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