vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.57k stars 660 forks source link

Introduce a repo just for stubs? #3347

Closed muglug closed 4 years ago

muglug commented 4 years ago

I'm wondering whether it would be worthwhile to have a repository just for machine-readable class and function stubs for common frameworks. A user would do something like composer require --dev psalm/framework-stubs and Psalm would be able use those in its analysis.

Any framework function/method whose return type can be expressed using templates or a conditional return type would be moved to that repo. All other custom functionality would remain with the plugins.

Pro:

Con:

cc @mr-feek @seferov @weirdan

muglug commented 4 years ago

The obvious parallel in the Typescript world is https://github.com/DefinitelyTyped/DefinitelyTyped

joehoyle commented 4 years ago

This sounds quite similar to the TypeScript approach to the @types namespace, which requires a PR to https://github.com/DefinitelyTyped/DefinitelyTyped, I think that has worked very well in TS, and I think it would be a great addition to Psalm to encourage more community contribution for stubs. Authoring plugins is a much larger undertaking!

mr-feek commented 4 years ago

I'm not necessarily on the side of maintaining another repo just for stubs as I'd need to give it more thought, but I'd like to mention another pro!

For the psalm laravel plugin, we will have to make many of the same stubs as the larastan phpstan plugin. It doesn't make much sense to be maintaining them in parallel, it would be great to have a single source of truth for stubs that all static analyzers could utilize.

erunion commented 4 years ago

I really like the idea of a single repo for stubs!

muglug commented 4 years ago

it would be great to have a single source of truth for stubs that all static analyzers could utilize.

Yeah, as Psalm and PHPStan support each others type annotations (or at least a decent subset thereof) they could share in this goodness.

muglug commented 4 years ago

for versioning I think there'd be some sort of structure like

- laravel/laravel/latest/model.phpstub
- laravel/laravel/6.x/model.phpstub
- laravel/laravel/shared/relations.phpstub
weirdan commented 4 years ago

Not convinced this is a better approach comparing to what we have already.

I think we need to answer these question before we commit to any solutions:

  1. What prevents people from contributing their own types using the plugin ecosystem? Probably we can simplify plugin scaffolding even further, by making current plugin skeleton a template repository.
  2. What prevents people from using existing plugins? Do people even know what plugins exist? I think we can state that there are already some plugins available more prominently in Psalm's docs (both on psalm.dev and in the README). Including the (automatically updated) list of available plugins may help as well. This should probably the next topic after installation, as that's probably the next thing end user needs.
muglug commented 4 years ago

Somebody will have to manage that repository - merging pull-requests, making releases, responding to issues.

I think the definitelytyped repo has some sort of auto-pull-request-approval thing?

What would be the criteria for making a new release?

That could happen on a scheduled cadence – once a week or something, with hotfixes done manually

How would we ensure even a minimal quality of submissions?

We could have some sort of test suite, but that's a problem (I think) that sort of disappears the more people use the repo.

How do we declare dependency on Psalm version?

With Composer, right? There would presumably some changes that required bumping the minimum Psalm version at some point

Everyone can make a Psalm plugin now, without asking any permissions. Monorepo is step backwards in that regard.

This wouldn't replace plugins – at least, that's not my intent. My intent was to separate some of the (very useful) custom behaviour that plugins provide out from the more basic behaviour (stubs), because I have this notion that stubs are sort of less messy.


Another thing to consider: many packages where a plugin was very necessary (e.g. assertion frameworks) have now added Psalm-compatible docblocks so plugins aren't necessary. Maybe the time for a centralised repository being necessary has passed.

What prevents people from using existing plugins? Do people even know what plugins exist? I think we can state that there are already some plugins available more prominently in Psalm's docs (both on psalm.dev and in the README)

Yes – I'd welcome PRs there. I think Psalm could go beyond that too, by suggesting a composer require --dev command that would install relevant plugins (i.e. a map of composer packages to Psalm plugins would be part of the app itself)

mr-feek commented 4 years ago

I'm not a typescript user, so I'm not familiar with DefinitelyTyped.

What is the rationale behind including stubs for all of the frameworks in one repo? Off the top of my head, I would think it's a lot more maintainable to have a separate repo of stubfiles per framework.

JanTvrdik commented 4 years ago

image

Currently we have:

1) stubs in https://github.com/php/php-src/ 2) stubs in https://github.com/JetBrains/phpstorm-stubs 3) stubs in https://github.com/phpstan/phpstan-src/tree/master/stubs 4) stubs in https://github.com/vimeo/psalm/tree/master/src/Psalm/Internal/Stubs

JanTvrdik commented 4 years ago

that's a problem (I think) that sort of disappears the more people use the repo.

It does not. Both DefinitelyTypes repo and many of the @types repositories suffer from having incorrect and/or out-of-date types. You would need a good tooling to avoid this issue.

Would the stubs include all public/protected properties/methods? Or only those with changed signature?

muglug commented 4 years ago

stubs in https://github.com/php/php-src/

To be clear, these stubs would just be for third-party PHP frameworks that are missing generic annotations that either haven’t added them yet (e.g. Laravel) or have, but in versions people might not upgrade to for a year (e.g. PHPUnit)

It does not. Both DefinitelyTypes repo and many of the @types repositories suffer from having incorrect and/or out-of-date types.

Yeah, and this would definitely have fewer users, so the quality would be lower.

weirdan commented 4 years ago

stubs in https://github.com/php/php-src/ stubs in https://github.com/JetBrains/phpstorm-stubs stubs in https://github.com/phpstan/phpstan-src/tree/master/stubs stubs in https://github.com/vimeo/psalm/tree/master/src/Psalm/Internal/Stubs

None of those are for libraries / frameworks though. So there's no overlap.

Overall, however, I don't see much difference between making a PR against https://github.com/psalm/psalm-plugin-laravel or https://github.com/psalm/framework-stubs.

joehoyle commented 4 years ago

I thought I'd add my 2c here, as I have some experience with TypeScript. I also have a good amount of experience in the WordPress open source ecosystem, and trying to encourage contributions to that project. However, I'm very new to Psalm, so feel free to disregard!

It's my assumption that the stubs repo is primary about defining richer type information for third party libraries. Getting stubs for the PHP functions / classes and coarse type information is already simple enough. Any project will get that via its composer deps that Psalm already can already analyze. I think the more valuable use-case is to allow community contribution of Psalm type annotations (like templates/generics, properly documented object-like-array keys etc).

If I'm writing code using totallyTyped=true, it's quite unlikely I'm getting rich enough type inference from any 3rd party libraries, so I'm likely to want to write some "handcrafted" overrides for functions in the library. This is the use-case that I think wants to be easy to share, and contribute back to the Psalm community. Creating a Psalm-plugin for a random dependency I use is extremely unlikely; and I imagine it would be similar for others. Not only do I need to go through the whole project setup, publishing to packagist, I need to own and maintain such a project.

A centralized stubs repo that can make it easier for developers who use Psalm to contribute back any type annotations they need to specify to their dependencies would significantly reduce the barrier to sharing with the rest of the community. I think this would be a significant undertaking, but I think it has potentially huge upsides. Capitalizing on (or leveraging) the existing work many Psalm users may already be doing (adding richer annotations to libraries) and providing a way to easily contribute it could lead to a network effect / positive feedback loop to get more use of Psalm and richer type information.

As it happens, I just went though the process of creating my first Psalm plugin. It was really great how much documentation, template repos and tooling I could take from existing projects (thanks!); but this was still a significant undertaking, both in terms of the work to get there, but the commitment I'll personally now need to support this project. I'm of the opinion that a centralized, easy to contribute to repo of stubs; with potentially out of date/incomplete stubs is better than no stubs at all. And I fear with the "Go write a Psalm plugin" approach, we'll essentially be in the "no stubs at all" scenario.

ondrejmirtes commented 4 years ago

This wouldn't be a problem if those 3rd party libraries had correct PHPDocs in the first place. So instead of sending PR to "PHP-DefinitelyTyped", you'd be sending PR to those libraries. If some libraries notoriously rejects those better type information (like Laravel), we could have a common repo just for that framework. But I think the majority of maintainers would actually like those PRs. (Not counting abandoned libraries which no one should use anyway.)

joehoyle commented 4 years ago

That’s true, though I think a lot of the benefit of Psalm is the more advanced annotations. Maybe a library will accept a PR for psalm-specific comments but I’m not sure I’d rely on it much. For someone who is a library author who has never heard of Psalm, I’m not sure how much of a given it is that they would be interested in accepting such PRs. Even something like the several array syntaxes that have been adopted by a few projects are still not (I think) compatible with PhpDoc array syntaxes.

I think that means it often ends up being the “lowest common denominator” problem where it’s only possible to use the most basic syntax and type information.

muglug commented 4 years ago

One other benefit of having a central repo is that even if the library author accepts the PR, the annotations might not be available to the submitter unless they upgrade, which can be its own headache.

ALSO plugins tend to be PHP-specific. [In an ideal world where PHPStorm supports these annotations] this centralised repo could be utilised by PHPStorm's Java-based PHP interpreter, by noverify and anyone else who's interested.

weirdan commented 4 years ago

One other benefit of having a central repo is that even if the library author accepts the PR, the annotations might not be available to the submitter unless they upgrade, which can be its own headache.

Actually it's a benefit of having typings separate from the libraries. Doesn't have to be a monorepo.

To clarify, it's the monorepo management where I see much potential pain. Individual packages could be aggregated in a central repo as git submodules, or listed in central registry by querying a keyword/tag on packagist. Having simple, stub only packages (in separate repos) seems better alternative to me. Especially if they can be scaffolded with ease. Ideally you would run, say, a cli tool with a package name as an argument and it would generate all the necessary files, ready to be pushed (I'm aware of psalm --generate-stubs, but it's not quite there yet).

Here's what I think could be an alternative approach:

Basically, I would prefer more distributed solution which I believe should lead to less load on any given individual (on average).

muglug commented 4 years ago

Other prior art:

Python has a repo for both stdlib and third-party library definitions: https://github.com/python/typeshed/tree/master/third_party/2and3

Sorbet (for Ruby) has sorbet-typed: https://github.com/sorbet/sorbet-typed/tree/master/lib

muglug commented 4 years ago

Basically, I would prefer more distributed solution which I believe should lead to less load on any given individual (on average).

if we give the repo a lot of maintainers (anyone who's created a Psalm plugin, and other static analysis tool authors) in a repo then there isn't such a heavy burden. I imagine there'd be a bunch of interested parties to help with maintenance (maybe that's wishful thinking, but I have faith).

I think it would be really nice if Psalm came with builtin stubs for a bunch of popular packages via a composer dep, and all people had to do get the latest definitions for pretty much every package they need would be to run composer update.

mr-feek commented 4 years ago

As far as maintenance would be concerned, would there be a minimum set of requirements for accepting a new package into the central monorepo? I'm not sure how the likes of typescript manages that

muglug commented 4 years ago

closing as this is a big-ticket item I don't actually think I have the energy to push through