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

Introduce isSuperTypeOfWithReason #3538

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

VincentLanglet
Copy link
Contributor

As suggested by #3536 (comment)

Comment on lines 133 to 136
public function negate(): self
{
return new self($this->result->negate(), $this->reasons);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dunno if it makes sens to have a negate method (Should the reasons be lost ?)

I created one since I had multiple Trinary::negate calls to replace.

Comment on lines 288 to 299
return TrinaryLogic::createNo()->lazyOr($otherType->getTypes(), fn (Type $innerType) => $this->isSubTypeOf($innerType));
return AcceptsResult::createNo()->or(...array_map(fn (Type $innerType) => $this->isSubTypeOfWithReason($innerType), $otherType->getTypes()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we're loosing a lazyOr into a or.

I dunno if the lazy could be kept on AcceptsResults but I saw similar lazy lost when isAcceptedWithReasonBy was introduced

Comment on lines 233 to 238
return TrinaryLogic::createYes()->lazyAnd($this->getTypes(), static fn (Type $innerType) => $innerType->isSuperTypeOf($otherType));
return AcceptsResult::createYes()->and(...array_map(static fn (Type $innerType) => $innerType->isSuperTypeOfWithReason($otherType), $this->types));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we're loosing a lazyAnd into a and.

I dunno if the lazy could be kept on AcceptsResults but I saw similar lazy lost when isAcceptedWithReasonBy was introduced

Comment on lines 242 to 252
$result = TrinaryLogic::lazyMaxMin($this->getTypes(), static fn (Type $innerType) => $otherType->isSuperTypeOf($innerType));
$result = AcceptsResult::maxMin(...array_map(static fn (Type $innerType) => $otherType->isSuperTypeOfWithReason($innerType), $this->types));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we're loosing a lazyMaxMin into a maxMin.

I dunno if the lazy could be kept on AcceptsResults but I saw similar lazy lost when isAcceptedWithReasonBy was introduced (In this same class for instance).

}

$result = TrinaryLogic::createNo()->lazyOr($this->getTypes(), static fn (Type $innerType) => $innerType->isSuperTypeOf($otherType));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same, lazyOr into or.

}

return $result;
}

public function isSubTypeOf(Type $otherType): TrinaryLogic
{
return TrinaryLogic::lazyExtremeIdentity($this->getTypes(), static fn (Type $innerType) => $otherType->isSuperTypeOf($innerType));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same lazyExtremeIdentity into extremeIdentity, like it's done for isAcceptedWithReasonBy

@ondrejmirtes
Copy link
Member

This looks promising. Could you do a quick proof of concept where this might be useful?

In StrictComparisonOfDifferentTypesRule we could read the reasons and output them as a tip (similar to what places that call Type::acceptsWithReason() do). A great opportunity for that is MixedType::isSuperTypeOfWithReason() when there's a subtracted type. So instead of mixed~string === string is always false, the error message would stay the same, but additionally there'd be a tip about "Type string has been eliminated from mixed already."

@ondrejmirtes
Copy link
Member

I understand it might be a bit challenging because we might have to propagate that all the way from initializerExprTypeResolver->resolveIdenticalType.

@ondrejmirtes
Copy link
Member

Maybe better to leave it for later, so this can be merged. So nevermind for now, I'll review this PR.

@VincentLanglet
Copy link
Contributor Author

Maybe better to leave it for later, so this can be merged. So nevermind for now, I'll review this PR.

Yes I had in mind to work on it step by step ^^'

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Hey, I was thinking about this a bit more and it'd help me if:

  1. We didn't reuse AcceptsResult for the isSuperTypeOfWithReason return type.
  2. This PR did not do any changes to AcceptsResult.
  3. Instead, it introduced new class called IsSuperTypeOfResult by copying and modifying AcceptsResult to suit the method's needs.

After that I'm very likely to merge this.

@VincentLanglet
Copy link
Contributor Author

Since I created isSubtypeOfWithReason too, does it mean we need a IsSubtypeOfResult too @ondrejmirtes ?

This may complicate things since i need to trasnform often subtypeOfResult into superTypeOfResult.

@ondrejmirtes
Copy link
Member

@VincentLanglet It's fine to use IsSuperTypeOfResult for isSubtypeOf too.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Oct 6, 2024

  1. We didn't reuse AcceptsResult for the isSuperTypeOfWithReason return type.
  2. This PR did not do any changes to AcceptsResult.
  3. Instead, it introduced new class called IsSuperTypeOfResult by copying and modifying AcceptsResult to suit the method's needs.

I'll end with something very similar for AcceptsResult and IsSuperTypeOfResult.
They will have the same and, or, ... methods.

I was wondering what's the idea between duplicating such code ; wouldn't it better to introduce something like a Result class which works for everything ?

It would make sens because there is code like

public function acceptsWithReason(Type $type, bool $strictTypes): AcceptsResult
	{
		if ($type instanceof CompoundType && !$type instanceof self) {
			return $type->isAcceptedWithReasonBy($this, $strictTypes);
		}

		return $this->isSuperTypeOfInternal($type, true);
	}
	
public function isSuperTypeOfWithReason(Type $type): IsSuperTypeOfResult
	{
		if ($type instanceof CompoundType && !$type instanceof self) {
			return $type->isSubTypeOfWithReason($this);
		}

		return $this->isSuperTypeOfInternal($type, false);
	}

So both method rely on isSuperTypeOfInternal and could use the same value object as return type.

Another "weird" situation is the fact that isParametersAcceptorSuperTypeOf currently returns an AcceptsResult when the method talk about SuperTypeOf...

public static function isParametersAcceptorSuperTypeOf(
CallableParametersAcceptor $ours,
CallableParametersAcceptor $theirs,
bool $treatMixedAsAny,
): AcceptsResult

So I'm not sure how to evolve this PR with the use of a IsSuperTypeOf without using a lot of method like
IsSuperTypeOfResult::toAcceptsResult and AcceptsResult::ToIsSuperTypeOfResult.

@ondrejmirtes
Copy link
Member

I was wondering what's the idea between duplicating such code ; wouldn't it better to introduce something like a Result class which works for everything ?

This is accidental duplication. The fact these are the same right now doesn't mean they're going to be the same in the future. I tend to develop a hunch for these things.

@ondrejmirtes
Copy link
Member

Another "weird" situation is the fact that isParametersAcceptorSuperTypeOf currently returns an AcceptsResult when the method talk about SuperTypeOf...

This method should return IsSuperTypeOfResult in your PR, because it's used in isSuperTypeOfInternal methods.

@ondrejmirtes
Copy link
Member

Hey, I need this really soon so I can announce an RC period for 2.0. I'm changing this locally right now.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Oct 7, 2024

Hey, I need this really soon so I can announce an RC period for 2.0. I'm changing this locally right now.

I started the changes, do you want me to stop and letting u doing all the changes then ? (Or I push the current changes ?)

@ondrejmirtes ondrejmirtes merged commit 8e9a34c into phpstan:1.12.x Oct 7, 2024
78 of 93 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

@VincentLanglet
Copy link
Contributor Author

Thanks you too !

@VincentLanglet VincentLanglet deleted the withReason branch October 7, 2024 08:18
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

2 participants