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

#463: psalm-specific type declarations for generic/intersection types of proxies #465

Merged
merged 26 commits into from
Jun 3, 2019

Conversation

Ocramius
Copy link
Owner

@Ocramius Ocramius commented May 23, 2019

Fixes #463

This is an incomplete patch that just serves to display that types can be
improved massively in here, making even this magic contraption of a library
usable downstream with some decent type constraints.

TODOs:

  • need to implement CI steps to ensure type inference works as expected
  • full type coverage in ProxyManager internals (note: no tests yet - those require more work)
  • pin to stable vimeo/psalm (^3.4.0)
  • split type check test file into multiple type-checked examples
  • raise psalm reporting levels
  • add utilities for PHPStorm type inference

Blocked by:

…arations

This is an incomplete patch that just serves to display that types can be
improved massively in here, making even this magic contraption of a library
usable downstream with some decent type constraints.
@Ocramius Ocramius added this to the 2.4.0 milestone May 23, 2019
@Ocramius Ocramius requested a review from malukenho May 23, 2019 20:51
composer.json Outdated Show resolved Hide resolved
type-example.php Outdated Show resolved Hide resolved
@stof
Copy link

stof commented May 24, 2019

a PR fixing itself ? That's inception.

@Ocramius
Copy link
Owner Author

a PR fixing itself ?

What do you mean by that? I didn't catch the joke XD

@stof
Copy link

stof commented May 24, 2019

The description contains this:

Fixes #465

This reference is the PR itself.

@Ocramius
Copy link
Owner Author

Ha, fixed, thanks!

@Ocramius Ocramius self-assigned this May 25, 2019
To get everything green, we'll probably need this massive list of issues
fixed first:

 - [x] vimeo/psalm#1673
 - [x] vimeo/psalm#1674
 - [x] vimeo/psalm#1675
 - [ ] vimeo/psalm#1677
 - [ ] vimeo/psalm#1679
 - [ ] vimeo/psalm#1680
 - [ ] vimeo/psalm#1681
 - [ ] vimeo/psalm#1682
 - [ ] vimeo/psalm#1683
Ocramius added 6 commits June 2, 2019 12:06
Won't get to fix them anyway, so they are just noise.
…arations

This allows for templating `AccessInterceptorValueHolderInterface` directly,
and therefore having the same object type for the `ValueHolderInterface` and
the `AccessInterceptorInterface` methods.

Without this, psalm gets quite confused about what type the factory is supposed
to create.
@Ocramius
Copy link
Owner Author

Ocramius commented Jun 2, 2019

Checks took 0.66 seconds and used 153.505MB of memory
Psalm was able to infer types for 99.7795% of the codebase

This is 🔥

Ocramius added 3 commits June 2, 2019 15:04
The `callable` initializer has a different signature for `GhostObject`
and `LazyLoadingInterface`, sadly.
Revert this commit when we need to re-introduce phpstan, but until
generic/templated types are supported, phpstan is not viable for static
analysis for this particular project.
psalm.xml Outdated Show resolved Hide resolved
@Ocramius
Copy link
Owner Author

Ocramius commented Jun 2, 2019

Ok, so my latest changes lead to wild issues in Psalm:

ERROR: UndefinedMethod - tests/static-analysis/lazy-loading-value-holder.php:57:25 - Method ProxyManager\Example\VirtualProxy\Foo::sayhello does not exist
    echo $wrappedValue->sayHello();

There is no mention of ProxyManager\Example\VirtualProxy\Foo in that file, to this feels like a global multi-file shared state issue. Not sure how to reproduce it for psalm to consume.

@muglug
Copy link
Contributor

muglug commented Jun 2, 2019

this feels like a global multi-file shared state issue

Yeah something in examples is polluting stuff - I'll find and fix

@muglug
Copy link
Contributor

muglug commented Jun 2, 2019

Fixed in vimeo/psalm@0ad5769

@Ocramius
Copy link
Owner Author

Ocramius commented Jun 3, 2019

Hmm, since latest builds, psalm now fails with something like this on 7.4:

$ ./vendor/bin/psalm
PHP Fatal error:  Uncaught ParseError: syntax error, unexpected 'Fn' (T_FN), expecting identifier (T_STRING) in /home/travis/build/Ocramius/ProxyManager/vendor/vimeo/psalm/src/Psalm/Internal/Provider/ReturnTypeProvider/ArrayFilterReturnTypeProvider.php:64

@muglug I just need to pin this to a stable release with a green build, and then I can release here 👍

@Ocramius
Copy link
Owner Author

Ocramius commented Jun 3, 2019

I'll send a patch to fix the Fn issue btw 👍

EDIT: see vimeo/psalm#1726

@dkarlovi
Copy link

dkarlovi commented Jun 3, 2019

a PR fixing itself ? That's inception.

@Ocramius is just that good.

@Ocramius
Copy link
Owner Author

Ocramius commented Jun 3, 2019

Green! Now needs a release upstream and we can 🚢 \o/

@muglug
Copy link
Contributor

muglug commented Jun 3, 2019

Will hopefully release later today, barring any major impediments

@muglug
Copy link
Contributor

muglug commented Jun 3, 2019

@Ocramius
Copy link
Owner Author

Ocramius commented Jun 3, 2019

Thanks! That should be enough to do a release here, if all is green 🚀

@Ocramius Ocramius changed the title WIP: Introducing initial tentative stub for #463: psalm-specific type declarations Introducing initial tentative stub for #463: psalm-specific type declarations Jun 3, 2019
@Ocramius
Copy link
Owner Author

Ocramius commented Jun 3, 2019

Relevant CI tasks are GO! 👍

@Ocramius Ocramius merged commit 7a133e8 into master Jun 3, 2019
@Ocramius Ocramius deleted the feature/#463-add-psalm-generic-type-declarations branch June 3, 2019 16:41
@Ocramius Ocramius changed the title Introducing initial tentative stub for #463: psalm-specific type declarations #463: psalm-specific type declarations for generic/intersection types of proxies Jun 3, 2019
@Ocramius Ocramius mentioned this pull request Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants