Skip to content

Make contravariant inferences only from pure contravariant positions#27357

Merged
ahejlsberg merged 4 commits into
masterfrom
fixBivariantInferences
Sep 26, 2018
Merged

Make contravariant inferences only from pure contravariant positions#27357
ahejlsberg merged 4 commits into
masterfrom
fixBivariantInferences

Conversation

@ahejlsberg
Copy link
Copy Markdown
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
Comment thread src/compiler/checker.ts Outdated
@@ -1,3 +1,6 @@



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.

I think that these newlines are strange.

Comment thread src/compiler/checker.ts
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
Copy Markdown
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
Copy Markdown
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
@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