PHP 5.6 / 8.0: NewConstantDereferencing: add support for constant array / object dereferencing

[elazar]

This RFC brings static scalar expressions to the parser. This allows places that only take static values (const declarations, ... etc) to also be able to take static expressions.

Constants... are currently special cased and only dereferencable under the [] operator (and do not even support the nominally equivalent {} operator).

This RFC proposes to make constants (and class constants) fully array and object dereferencable, i.e. to allow both FOO{0} and FOO->length() (the latter only being relevant in conjunction with scalar objects).

Ref:

• https://wiki.php.net/rfc/const_scalar_exprs
• php/php-src@d5ddd2d
• https://wiki.php.net/rfc/variable_syntax_tweaks#constant_dereferencability

Includes unit tests.

Relates to #809.

[jrfnl]

Hi @elazar Finally... finally I've had a chance to properly look at this PR... sorry for the long wait.

There's quite a lot to unpack here, so if you like, I'd be happy to talk things over in a call.

About the PHP 5.6 change:

As this change is related to the "constant arrays using const keyword" change (also see the InitialValue.NewConstantArraysUsingConst sniff), the question arises whether this change applies to global constants only or also to namespaced constants and class constants.
If the latter (which I believe is the case, but will need to be confirmed via 3v4l or otherwise), unit tests will need to be added to cover this.

Curly braces

and do not even support the nominally equivalent {} operator

I wonder if that is correct. The PHP 7.4 curly braces sniff Syntax.RemovedCurlyBraces does seem to check for constant array access using curly braces, but only for global constants, not for namespaced or class constants.

So, again, I think some checking is needed whether curly braces were supported and under what circumstances and the tests should document the findings.

register()

Currently the sniff targets the T_OPEN_SQUARE_BRACKET, T_OPEN_CURLY_BRACKET and the T_OBJECT_OPERATOR tokens.
Those are tokens with a very high usage frequency. While T_STRING also occurs very often in code, I wonder whether it would be more efficient to use that as the base token to sniff from.

Side-note (not for this PR, more a reminder for later): in PHP 8 the tokens for namespaced names have changed. The next version of PHPCSUtils will contain some utilities for this and once it is out, this sniff will probably need to use those.

Another side-note: I've just checked, but as far as I can see based on unit tests in PHPCSUtils, constant dereferencing does not appear to be affected by tokenizer bugs tokenizing the T_OPEN_SQUARE_BRACKET as T_OPEN_SHORT_ARRAY, so it seems we're good on that front.

process()

There is a Sniff::isUseOfGlobalConstant() method in the PHPCompatibility\Sniff class which might be helpful.
If class constants/namespaced constants would also need to be supported, it might be worth looking into splitting that method up into smaller parts to allow for detecting those situations too.

Some of the logic used in this sniff will have overlap with logic in the Syntax.RemovedCurlyBraces sniff, particularly in the isConstantArrayAccess() method.
It might be worth looking into consolidating the overlapping logic, in a similar way as is done for other overlapping logic between the Syntax.RemovedCurlyBraces sniff and, for instance, the NewArrayStringDereferencing or NewClassMemberAccess sniffs.

Nitpicks to address at some point

• Beware of very long lines
• Might be worth it to bow out early at the top if PHP 7.4 or lower does not need to be supported.
• Preferably use isset() with token constants in the keys and irrelevant values over in_array().

[elazar]

@jrfnl

About the PHP 5.6 change:

I confirmed that this change does apply to namespace and class constants.

Namespace constants: https://3v4l.org/kUHCe
Class constants: https://3v4l.org/qVT4E

Curly braces

I confirmed that this change does NOT apply: curly braces on namespace and class constants produce a parse error in PHP < 8 and a fatal error in PHP >= 8.

Namespace constants: https://3v4l.org/9CU6u
Class constants: https://3v4l.org/h1WJc

I think I've integrated all other feedback (please let me know if I missed anything) with the exception of refactoring and/or integrating Sniff::isUseOfGlobalConstant() and/or Syntax.RemovedCurlyBraces::isConstantArrayAccess(). Let me know if, based on my current changes, you still think that should be pursued, as it will take some digging to pull apart the code and figure out the best way to do so.

Thanks!

[jrfnl]

Hi Matthew, thanks for verifying the basis of the sniff.

I've gone through the sniff with a fine toothcomb now and left quite some comments in line, mostly just small touching up.
On the note of "small touching up": please use FQNs for global constants and function calls.

The more pregnant issues are the following:

public function addMatcher(MatcherInvocation $matcher): void {} // <- False positive.
class ORM implements \ArrayAccess {} // <- False positive.

echo 'print ', CONSTANT_NAME['foo']; // <- False negative.
echo 'print ', ClassName::CONST_NAME->bar; // Correctly detected.
echo 'print ', \NS_CONST[0]; // Correctly detected.

echo \Foo\BAR{0}; // <- False positive. Syntax is not supported in PHP, so out of scope of this sniff.
echo \Foo::BAR{0}; // <- False positive. Syntax is not supported in PHP, so out of scope of this sniff.

The majority of these issues can be fixed by adding an additional check to the "is this a reference" condition, like so:

if (!isset($referenceTokens[$nextNonEmptyCode])
    || ($nextNonEmptyCode === T_OPEN_CURLY_BRACKET && isset($tokens[$nextNonEmpty]['scope_opener']) === true)
) {
    return;
}

The T_COMMA can then also be removed from the $outOfScopeTokens array, as well as quite a few other tokens.

I think it would also be good to add some tests with "double array dereferencing" to safeguard against potential upstream tokenizer issues:

echo FOO[0]['foo'];
echo \Foo\BAR['foo'][1];
echo \Foo::BAR[20]['bar'];

[jrfnl]

Can this array be turned into a class property ? and as a property possibly given a value which would be helpful to do the check on line 114 and 126 ? (i.e. which error should be thrown for what)

[elazar]

The array here contains T_OBJECT_OPERATOR in addition to the other two tokens also used on 126. That would leave us doing something like this here:

if (!isset($this->referenceTokens[$nextNonEmptyCode])
    && $nextNonEmptyCode !== \T_OBJECT_OPERATOR) {

Is that what you had in mind?

[elazar]

@jrfnl Think I've resolved most your issues and left questions on the others. Let me know your thoughts and hopefully we can make the next round of feedback the last for this PR. Thanks!