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

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

Closed
bmewburn opened this issue May 22, 2020 · 17 comments

Comments

@bmewburn
Copy link

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?

@mr-feek
Copy link
Contributor

mr-feek commented May 23, 2020

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

@mfn
Copy link
Collaborator

mfn commented May 25, 2020

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[]

@mr-feek
Copy link
Contributor

mr-feek commented May 25, 2020

@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: larastan/larastan#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!

@mfn
Copy link
Collaborator

mfn commented May 25, 2020

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? 🤔

@mr-feek
Copy link
Contributor

mr-feek commented May 25, 2020

@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.

@bmewburn
Copy link
Author

@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.

@mfn
Copy link
Collaborator

mfn commented May 26, 2020

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.

@mr-feek
Copy link
Contributor

mr-feek commented Jul 10, 2020

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

@mfn
Copy link
Collaborator

mfn commented Jul 10, 2020

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

@spawnia
Copy link
Contributor

spawnia commented Dec 10, 2020

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?

@mfn
Copy link
Collaborator

mfn commented Dec 10, 2020

More configurations, more conditions in an already spaghetti code 😥

@MaxKorlaar
Copy link

MaxKorlaar commented Aug 3, 2021

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/

@mfn
Copy link
Collaborator

mfn commented Aug 6, 2021

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

@miken32
Copy link
Contributor

miken32 commented Sep 24, 2021

@nevmerzhitsky
Copy link

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.

@jnoordsij
Copy link

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

@mfn
Copy link
Collaborator

mfn commented Feb 1, 2023

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 mfn closed this as completed Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants