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

Issue with template inheritance and overriding method signatures #10010

Closed
wants to merge 1 commit into from

Conversation

boesing
Copy link
Contributor

@boesing boesing commented Jul 9, 2023

I have no clue how to describe the issue, maybe I am using something wrong or I don't know.
I dived through the code and found out that this code would work if the following check would return true:

\Psalm\Internal\Codebase\Methods::getMethodReturnType

$new_contained_by_old = UnionTypeComparator::isContainedBy(
    $source_analyzer->getCodebase(),
    $overridden_storage_return_type,
    $candidate_type,
);

The union comparator returns false due to the fact that the TTemplateParam TRequestedInstance provides mixed but should return a super of InstanceType due to TRequestedInstance extends InstanceType and InstanceType is actually known as PluginInterface.

So I guess I do have to find out why the TRequestedInstance is not properly inferred as PluginInterface as that would probably lead to the expected result.

Fun fact, the code works once removing the overriden method AbstractSingleInstancePluginManager#get as it narrows the return value from mixed to object which then seems to mess up the whole type detection...

Happy if some1 has some little guidance here and can point me in the correct direction. As of now, I'd say the problem is that TRequestedInstance is not providing PluginInterface as it should, but no clue where this should happen.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
Copy link
Contributor Author

boesing commented Jul 17, 2023

@orklah @weirdan any ideas? :-/

@orklah
Copy link
Collaborator

orklah commented Jul 18, 2023

I'll try to take a look in the next few days :)

interface PluginManagerInterface
{
/**
* @template TRequestedInstance extends InstanceType
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we support extends here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it works flawless if I do not overwrite the method in the abstract class. the issue is adding the object return type which makes it less specific.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I confirm we don't do anything with the extends keyword so it's just comments as far as Psalm is concerned

Copy link
Contributor Author

@boesing boesing Jul 23, 2023

Choose a reason for hiding this comment

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

but it actually seems to work until I add the overridden method with the object return type 🤔 or its just a coincdedence as I do not verify some where if that is actively validated.

So to implement this, I have to focus the extends?

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you show an example where psalm behaves differently based on the extends?

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 guess I cant. I just know that the way psalm behaves differs once I extend the get method and narrow the type from mixed to object

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you post two snippets so we see the difference exactly? I'm still a bit confused

Copy link
Contributor Author

@boesing boesing Jul 25, 2023

Choose a reason for hiding this comment

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

https://psalm.dev/r/e80a63390b
https://psalm.dev/r/442b189bf4

But I am pretty sure that it does not respect the extends within the template annotation in method scope but not sure if that is the underlying problem tho.

Copy link
Contributor Author

@boesing boesing Aug 27, 2023

Choose a reason for hiding this comment

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

Okay, I've digged a bit deeper and found out that there is a general issue with @inheritdoc as due to this, templates are completely dropped and therefore the whole type detection seems to be problematic when it comes to assertions.

https://psalm.dev/r/538a0d30d4

I am totally lost here, but thats maybe due to the fact that I spent too much time on other psalm related stuff in the past days.
Would really love to get some help on this but will also try to solve this on my own in the next weeks.

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