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

Short list vs short array #1984

Closed
jrfnl opened this issue Apr 10, 2018 · 11 comments
Closed

Short list vs short array #1984

jrfnl opened this issue Apr 10, 2018 · 11 comments

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Apr 10, 2018

PHP 7.1 introduced the "short list syntax", i.e. using the short array syntax instead of calling list().
Ref: http://php.net/manual/en/migration71.new-features.php#migration71.new-features.symmetric-array-destructuring

Now I can imagine that in some situations it would be useful to be able to make the distinction whether something is used as a short list vs a short array.

There are two ways this can be done:

  1. Introduce T_OPEN_SHORT_LIST/T_CLOSE_SHORT_LIST tokens and let the Tokenizer handle it.
    The downside of this first solution is that in a lot of cases - especially when sniffing for code style -, I imagine that short list/array should be treated the same, so those sniff would then need to add two additional tokens to sniff for.
  2. Introduce a utility function which can make the distinction and can be called by select sniffs if/when needed.

For the PHPCompatibility standard, I have already created the utility function (solution 2), including extensive unit tests for it.
See: PHPCompatibility/PHPCompatibility#635

If there would be interest in having such a utility function in PHPCS itself, I'd be happy to pull it here.

@gsherwood
Copy link
Member

I would definitely prefer the utility function over the tokens. But I'm worried about including it without any built-in sniffs using it because it makes it much harder for me to maintain. But I could probably get over that with the tests.

But the bigger problem is that I wouldn't merge in the function you built for PHPCompat if it contains code to keep it working in unsupported PHPCS versions. So I don't think that putting this util method in will help with what you're doing because you need your version anyway. Happy to be wrong though.

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 13, 2018

But the bigger problem is that I wouldn't merge in the function you built for PHPCompat if it contains code to keep it working in unsupported PHPCS versions.

The version I would pull here would not contain those back-compat fixes. ;-)

you need your version anyway

True, we would for the PHPCompatibility lib, but I imagine there will be other external standards which don't support PHPCS versions that far back which could benefit from the utility function.

@jrfnl
Copy link
Contributor Author

jrfnl commented May 8, 2018

@carusogabriel That's completely irrelevant and besides the point of this issue.

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 15, 2018

I will address this as part of #2189

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 20, 2021

The more work I've done on this, the more convinced I become this should be solved at a tokenizer level.

As lists can be nested in arrays and arrays nested in list (keys), determining whether something is a short list or a short array is not as easy as it sounds, except for the top level.
So for a nested short list/short array you more often than not have to walk all the way to the top-level array/list to determine what it is, which for large arrays can be a huge performance drain. Especially if the same processing has to be done in multiple sniffs.

Either tokenizing short lists as T_OPEN/CLOSE_SHORT_LIST or having a nested_square_brackets key (array with open => close bracket pointers) added to the token array would help immensely.

I'd like to propose implementing either one of these solutions (or both) in PHPCS 4.x.

@gsherwood I'm willing to do the work for it, including the tests, if you could give me guidance on which of these proposals would be preferred.

Note: this would also allow for solving the bug in the DisallowShortArray sniff which currently auto-fixes short lists to array() syntax.

@gsherwood
Copy link
Member

I've been learning towards an index for nested square brackets + a util function mostly so that supporting this syntax is optional, but 4.0 is the time to introduce tokens if they are needed.

So having thought about it more, I think that new tokens is the way to go, and then change over sniffs that look for T_OPEN_SHORT_ARRAY to also look for T_OPEN_SHORT_LIST if they want to apply the same rules.

What were you thinking for this one?

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 22, 2021

Well, having the index for nested square brackets would be an awesome stepping stone and would allow for resolving short list vs array so much more easily, so just having that would be enough.

Anything else are nice to have extras.

What sort of util function for this were you thinking of ?

@gsherwood
Copy link
Member

What sort of util function for this were you thinking of ?

Just one to determine if a T_OPEN_SHORT_ARRAY is being used for list syntax or not, rather than having sniffs duplicate the logic.

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 22, 2021

Just one to determine if a T_OPEN_SHORT_ARRAY is being used for list syntax or not, rather than having sniffs duplicate the logic.

Makes sense. If the nested square brackets index is added, that should be easy enough to do anyway, but definitely useful to have a utility function for it.

@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 2, 2023

Closing as replaced by PHPCSStandards/PHP_CodeSniffer#12

@jrfnl jrfnl closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for Release
Development

Successfully merging a pull request may close this issue.

3 participants