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

Change doc-string for Unreliable variance flag. #33036

Conversation

jack-williams
Copy link
Collaborator

Minor documentation change that may be helpful to distinguish unreliable/unmeasurable.

@@ -4214,7 +4214,7 @@ namespace ts {
Independent = 1 << 2, // Unwitnessed type parameter
VarianceMask = Invariant | Covariant | Contravariant | Independent, // Mask containing all measured variances without the unmeasurable flag
Unmeasurable = 1 << 3, // Variance result is unusable - relationship relies on structural comparisons which are not reflected in generic relationships
Unreliable = 1 << 4, // Variance result is unreliable - relationship relies on structural comparisons which are not reflected in generic relationships
Unreliable = 1 << 4, // Variance result is unreliable - checking may produce false negatives, but not false positives
Copy link

Choose a reason for hiding this comment

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

I thought it was the reverse: The variance check may cause a typecheck to pass (positive) where the structural check would fail (negative)

#32664 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #33020 and the associated issue for variance checks raising errors where a structural comparison would not.

Choose a reason for hiding this comment

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

Hmm, I'm just confused because that seemingly contradicts the conversion I had with @weswigham (linked in my post above).

Choose a reason for hiding this comment

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

In particular:

Unreliable is supposed to be "fallback to structural if varianced-based check fails"

Which sounds like it shouldn't be able to cause an error only based on the variance result.

Choose a reason for hiding this comment

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

Also (I'm done after this I promise):

Only Unreliable rather than Unmeasurable since it should be that in any case where the variance result is wrong, it's because the variance is overstrict because we have more relaxed arity-based rules on the signature than tuples have.

Implying that any false negatives will be discarded in favor of the structural result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My reading:

The marker indicates that negative results are unreliable, therefore structural checks are used in the event of failure.

The positive results are assumed to be reliable such that false positives do not occur, even if in reality they can. There is no fallback in the event of success.

Choose a reason for hiding this comment

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

I guess when I hear "false positive" I think of, e.g. a virus scanner reporting a virus where none exists; likewise the unreliable variance can produce a successful typecheck where the canonical one (structural) would fail. We thus seem to be seeing the "positive" and "negative" from opposite sides of the fence, hence the confusion 😄

Reverse polarity! ➕ <--> ➖

@weswigham weswigham merged commit ec39d41 into microsoft:master Aug 23, 2019
@jack-williams jack-williams deleted the docstring-for-unmeasurable-unreliable branch August 24, 2019 11:37
timsuchanek pushed a commit to timsuchanek/TypeScript that referenced this pull request Sep 11, 2019
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.

None yet

3 participants