laravel-ide-helper icon indicating copy to clipboard operation
laravel-ide-helper copied to clipboard

Option for generating `Collection|ElementType[]` types as `Collection<int, ElementType>` instead

Open bmewburn opened this issue 4 years ago • 15 comments

Many static analysis tools now support generic notation for Iterator or ArrayAccess implementations. In this situation Collection|ElementType[] is not useful as it does not convey the real type and makes workarounds necessary within static analysis tools to correct for this. Is there an option to generate the stubs in generic notation? Or would it be possible to add an option?

bmewburn avatar May 22 '20 22:05 bmewburn

I’d be willing to help implement this if a PR would be welcome. I’ve been reverse engineering a lot of this logic for psalm

mr-feek avatar May 23 '20 14:05 mr-feek

We need to be careful because whilst this improves the static analysis tools, this will reduce usefulness of the typehints for IDEs. (*)

Remember: the primary goal is for IDEs to help here :-)

It's only recently that tools like https://github.com/nunomaduro/larastan embrace this information too and in fact expand upon it.

(*) PhpStorm being the most popular one, I know that this syntax Collection<int, ElementType> currently will not yield PhpStorm understanding the this is the same as ElementType[]

mfn avatar May 25 '20 14:05 mfn

@bmewburn I'm interested in what issue you are running into in which you need this option first-party in laravel-ide-helper.

In larastan, I wrote a parser that treats Collection|Model[] as Collection<Model> here: https://github.com/nunomaduro/larastan/pull/479

And psalm can infer these types by settingallowPhpstormGenerics='true' in your config https://psalm.dev/docs/running_psalm/configuration/#allowphpstormgenerics


@mfn I totally understand your point about this being primarily an ide-helper. I just wanted to point out that psalm has a LSP implemented, which means that (with the LSP plugin) phpstorm direcly benefits from the improvements we are making in static analysis tooling!

mr-feek avatar May 25 '20 17:05 mr-feek

phpstorm direcly benefits from the improvements we are making in static analysis tooling

I don't follow this as PhpStorm doesn't support this proposed syntax change? Or am I missing something? 🤔

mfn avatar May 25 '20 19:05 mfn

@mfn check this out: https://psalm.dev/docs/running_psalm/language_server/

It's pretty neat.


Don't mean to derail this thread. I think ide-helper should continue to write the phpstorm syntax, as devs shouldn't be forced to use the likes of psalm.

mr-feek avatar May 25 '20 19:05 mr-feek

@mr-feek my use case is in intelephense I was hoping to avoid any special handling of this type notation.

@mfn I agree it would have to be option and understand if it is out of scope of this project.

bmewburn avatar May 25 '20 22:05 bmewburn

I think until "the major IDE", which is the main benefactor of this whole library, catches up with the syntax, we can't change/introduce this syntax.

larastan has shown that's it's more feasible to simple adapt the static analyzers at the moment, as they can move faster here.

Unless I'm mistaken, for PhpStorm this would be the issue to vote on: https://youtrack.jetbrains.com/issue/WI-43843 (though it only talks about array<…> so I might be wrong).

There's also https://youtrack.jetbrains.com/issue/WI-20193 but I would rather say this one is superseded by the above one.

mfn avatar May 26 '20 03:05 mfn

it's been announced that phpstorm is going to have first party support for array key and value types, so it might be reasonable for us to switch (or at least include!) the Collection<int, ElementType> syntax

mr-feek avatar Jul 10 '20 03:07 mr-feek

Yep, let's wait until it lands how it works out!

mfn avatar Jul 10 '20 07:07 mfn

PhpStorm 2020.3 does indeed support array<TKey, TValue> syntax now. A generalized implementation of generics on iterables still isn't there yet.

That being said, I certainly prefer the technically correct annotation of Collection<T>. How about we add a configuration option for this?

spawnia avatar Dec 10 '20 17:12 spawnia

More configurations, more conditions in an already spaghetti code 😥

mfn avatar Dec 10 '20 20:12 mfn

Now that PHPStorm 2021.2 has better support for generics, it could be interesting to look into implementing this again.

https://blog.jetbrains.com/phpstorm/2021/07/phpstorm-2021-2-release/

MaxKorlaar avatar Aug 03 '21 07:08 MaxKorlaar

I would certainly agree, very well reflects what I said in https://github.com/barryvdh/laravel-ide-helper/issues/942#issuecomment-633596630 👍

mfn avatar Aug 06 '21 12:08 mfn

And still more support is coming in the next release.

https://blog.jetbrains.com/phpstorm/2021/09/phpstorm-2021-3-early-access-program-is-open/#more_for_generics

miken32 avatar Sep 24 '21 21:09 miken32

I would certainly agree, very well reflects what I said in #942 (comment) 👍

Well, is it planned to implement the subject? Sorry, I don't get your reference to your previous comment.

nevmerzhitsky avatar Nov 15 '21 10:11 nevmerzhitsky

I think this can be closed now that #1298 is merged

jnoordsij avatar Jan 31 '23 14:01 jnoordsij

Thanks!

Note: it's not released yet, so anyone finding this issue: if there's no release after v2.12.3 yet, you need to use the dev version or wait.

mfn avatar Feb 01 '23 10:02 mfn