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

PHPStan callable definitions do not support custom mapped types #224

Closed
jiripudil opened this issue May 29, 2019 · 12 comments
Closed

PHPStan callable definitions do not support custom mapped types #224

jiripudil opened this issue May 29, 2019 · 12 comments

Comments

@jiripudil
Copy link
Contributor

@jiripudil jiripudil commented May 29, 2019

Version: dev-master

Bug Description

The callable definitions introduced in #216 fail to support custom mapped types:

class MappedValues {
    public string $name;
}

$form = new Nette\Forms\Form();
$form>addText('name', 'Name');
$form->setMappedType(MappedValues::class);
$form->onSuccess[] = function (Form $form, MappedValues $values) {};

produces

Array (array<(callable(Nette\Forms\Form, array): void)|(callable(Nette\Forms\Form, Nette\Utils\ArrayHash): void)>) does not accept Closure(Nette\Forms\Form, MappedValues): void.

I don't know how the variance works there, but neither adding callable(Form, object) nor class MappedValues extends ArrayHash seemed to help. Perhaps @ondrejmirtes or @JanTvrdik could chime in please?

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor

@ondrejmirtes ondrejmirtes commented May 29, 2019

Hi, there's no way to support this. Form definitions and onSuccess are usually in different files... We should probably revert it to mixed. /cc @adaamz

@adaamz

This comment has been minimized.

Copy link
Contributor

@adaamz adaamz commented May 29, 2019

What about array and second variant for object? mixed is too wide...
Also we should somehow fix variant with only one parameter (Form $form): void

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor

@ondrejmirtes ondrejmirtes commented May 29, 2019

It will complain about dangerous calls on object on level 6+.

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor

@ondrejmirtes ondrejmirtes commented May 29, 2019

Also we should somehow fix variant with only one parameter (Form $form): void

Does not have to be fixed. It's fine to pass callable with a single parameter somewhere multiple arguments are passed.

@jiripudil

This comment has been minimized.

Copy link
Contributor Author

@jiripudil jiripudil commented May 29, 2019

What do you mean,by "revert it to mixed"? Isn't it the case that an array of callable(Form, object) (or callable(Form, mixed) for that matter) will never accept a Closure(Form, SpecificClass) because of LSP? The second argument type is not contravariant, so the closure is not a subtype of the callable type 🤔

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

@JanTvrdik JanTvrdik commented May 29, 2019

  • callable(Form, mixed) should accept Closure(Form, SpecificClass) because of gradual typing,
  • callable(Form, object) should not accept Closure(Form, SpecificClass)
@jiripudil

This comment has been minimized.

Copy link
Contributor Author

@jiripudil jiripudil commented May 29, 2019

callable(Form, mixed) should accept

Then there must be some issue there because, apparently, it does not

@JanTvrdik

This comment has been minimized.

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor

@ondrejmirtes ondrejmirtes commented Jun 1, 2019

Should be fixed: phpstan/phpstan#2170

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 7, 2019

What is the final consensus? mixed or object? (#223)

@jiripudil

This comment has been minimized.

Copy link
Contributor Author

@jiripudil jiripudil commented Jul 8, 2019

I believe mixed since object would not work as discussed above. I'll amend the PR

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor

@ondrejmirtes ondrejmirtes commented Nov 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.