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

Fix #6311 #6328

Closed
wants to merge 2 commits into from
Closed

Conversation

weswigham
Copy link
Member

By moving predicate argument position comparison to signature comparison, since the compatibility of isolated predicate comparisons never hinges on argument position equality.

Thoughts? @JsonFreeman @DanielRosenwasser


Edit by @DanielRosenwasser: Fixes #6311

}
const sourcePredicate = sourceReturnType as PredicateType;
const targetPredicate = targetReturnType as PredicateType;
if (sourcePredicate.predicate.kind === TypePredicateKind.Identifier) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't they both need to have the same predicate type?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@DanielRosenwasser
Copy link
Member

So the idea is that a predicate type will always be assignable to a boolean (and nothing else), but that when relating signatures, that's when we check for any meaning on them. Is that correct?

If so, I think this might be the way to go.

@@ -5666,7 +5655,24 @@ namespace ts {
}
}

return result & isRelatedTo(sourceReturnType, targetReturnType, reportErrors);
result = result & isRelatedTo(sourceReturnType, targetReturnType, reportErrors);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be a better idea to first check if they are both predicate types, since it doesn't look like you need to relate the return types in that case.

@JsonFreeman
Copy link
Contributor

I think this approach is moving in the right direction, which is pushing more of the comparison burden on the signature instead of the type predicate. That said, I am still in favor of completely obliterating the type predicate as a type, and doing the full comparison on a signature basis. I don't understand how it is meaningful to compare type predicates at all, given that their meaning is entirely based on their signature of origin.

@DanielRosenwasser
Copy link
Member

I think there are merits to the current approach. Namely:

  1. Representing predicates as types allows inferences to be made from predicates, which can be nice for filter-like functions.
  2. this-based type guards need not be attached to a signature, and that is easier to represent as a type. Keeping the two in sync is generally cleaner.

@JsonFreeman
Copy link
Contributor

I agree about the second point, regarding this predicates. I'm not so sure about the first, for two reasons:

  1. I think inferences can still be made from type predicates, even if they live on the signature.
  2. I'm not sure inferences should be made from type predicates, but this boils down to a deeper philosophical question about type argument inference. It's a question of whether you think all positions in the type system are fair game for type argument inference, versus just positions that can be inhabited by values at runtime. For example, clearly the type of a property or a return type is a good thing to infer from. But the type in a type predicate or the type of a parameter is more dubious because those types are invisible at runtime. So I suppose, to your point, as long as we infer from parameters, why should type predicates be any different?

@DanielRosenwasser
Copy link
Member

I think you're right that we could continue limiting to signatures, but I'd rather we went our course with how things are now, and revisit if there are other major issues that pop up.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 5, 2016

I do agree with @JsonFreeman that predicates do not belong in a type. this is leading to the current behavior. i am not sure i understand why something like this.thisPredicate() === argumentPredicate(a) is not allowed either..

@DanielRosenwasser
Copy link
Member

@mhegazy where are you getting that example from? I don't think we ever said that should be an error, and with this change, it shouldn't be considered an error.

@JsonFreeman
Copy link
Contributor

I think it could be result in an error even with this change, because the type portions of the type predicates are still being compared. So if the types are different, it will be an error, though in my opinion it should not be.

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.

5 participants