AbstractFunctionParameterSniff: fix first class callables and function imports

[rodrigoprimo]

The AbstractFunctionParameterSniff class was incorrectly flagging first class callables and function imports as a function call without parameters. This PR fixes this behavior by introducing the AbstractFunctionParameterSniff::is_targetted_token() method. This method calls the parent method and then performs two additional checks to make sniffs extending this class ignore first class callables and function imports.

Since there are no tests for the abstract classes and the plan is to replace those classes with similar PHPCSUtils classes, I opted to add a few tests to the I18nTextDomainFixer tests that generate false positives before the changes implemented in this PR.

Update 24/02/2025: after discussing this during the review process (see #2518 (comment)), this PR now also changes how the AbstractFunctionParameterSniff class handles unfinished function calls (when the closing parenthesis is missing). Now, it will ignore them as syntax error or live coding.

[jrfnl]

@rodrigoprimo Thank you for working on this.

I've verified the tests cover the change. ✔️,
Some remarks about the tests:

• Please do set a valid value for old_text_domain as currently the sniff would throw unrelated errors if it would be triggered.
• Please remember to reset the changed properties to their default value at the end of a test case file as otherwise it can influence other tests.
• Lastly, I see no reason for the tests to have their own case file. AFAICS these can just be added to the 3 or 4 file ?

As for the new method.... I believe the code in the is_targetted_token() can be simplified.
I also think the code as-is, is broken as it doesn't account for variable unpacking.
While the sniff used for testing wouldn't necessarily be able to act on that, it is not for the abstract to disregard variable unpacking outright.

Want to have another think about this yourself or would you like me to give you some clues ?

[rodrigoprimo]

Thanks for the review, @jrfnl!

Some remarks about the tests:
Please do set a valid value for old_text_domain as currently the sniff would throw unrelated errors if it would be triggered.
Please remember to reset the changed properties to their default value at the end of a test case file as otherwise it can influence other tests.
Lastly, I see no reason for the tests to have their own case file. AFAICS these can just be added to the 3 or 4 file ?

I moved the tests to I18nTextDomainFixerUnitTest.4.inc.

As for the new method.... I believe the code in the is_targetted_token() can be simplified.

I made some minor simplifications, but I have a feeling you have something else in mind that I could not find.

I also think the code as-is, is broken as it doesn't account for variable unpacking.
While the sniff used for testing wouldn't necessarily be able to act on that, it is not for the abstract to disregard variable unpacking outright.

I changed the code to disregard only first class callables and not variable unpacking.

Could you please take another look?

[jrfnl]

@rodrigoprimo Had another look. Second and third commit are useful, the fourth is definitely not what I had in mind.

Hint: Have a look at the "function import" check code - in the original method in the other abstract, it is important that it is determined properly whether something is a function import statement. For this method, however, we don't need to determine whether it is, just that it isn't a function call (which is much simpler to do and would use code which the first class callable check needs as well anyway).

I'm also missing a test safeguarding that the abstract will not disregard variable unpacking as if it were a first class callable.

[rodrigoprimo]

Thanks again, @jrfnl.

Hint: Have a look at the "function import" check code - in the original method in the other abstract, it is important that it is determined properly whether something is a function import statement. For this method, however, we don't need to determine whether it is, just that it isn't a function call (which is much simpler to do and would use code which the first class callable check needs as well anyway).

I believe now I have implemented something in line with what you hinted. I changed the first check in the method to only verify if the token after the function name is not an opening parenthesis instead of checking the previous and next tokens to ensure it is not a function import.

I'm also missing a test safeguarding that the abstract will not disregard variable unpacking as if it were a first class callable.

I added it now. At first, I didn't include this test as it triggers a false positive in the sniff that must be addressed separately. But it makes sense to have it to ensure that the changes to the abstract behave correctly. I will create a separate issue to check if the sniffs that extend this abstract need to be updated to handle variable unpacking correctly.

[jrfnl]

@rodrigoprimo Functionally, all is good now.

Code-style wise, I'm not keen on the implicit comparisons in the code, like if ($firstNonEmpty), as they are very bug prone.
They will work in this case as you're looking for findNext(), but can easily break on a findPrevious() where the result can be a perfectly valid 0.
I'd much prefer if the conditions are made explicit - if ($firstNonEmpty !== false) - to make it clearer what is actually being checked and to stabilize the code.

On that note: the check for T_OPEN_PARENTHESIS does not check against $next being falsy, while the other two findNext() calls do.
Question: are the "falsy" checks even necessary ? Keep in mind that these are function names which have already been validated by the parent::is_targetted_token() function...

I also think the tests could be improved a little more too as there are a number of things not tested now, but it could be argued that that should be left for the abstract in PHPCSUtils.
Situations which the code currently handles correctly, but which are not currently safeguarded by the tests:

• Live coding (those implicit conditions) x 2.
• Whitespace and comments in unconventional places, like between the function name and the open parenthesis, between the open parenthesis and the ..., between the ... and the close parenthesis and/or variable.

[rodrigoprimo]

Thanks for checking the changes, @jrfnl!

Code-style wise, I'm not keen on the implicit comparisons in the code, like if ($firstNonEmpty), as they are very bug prone.
They will work in this case as you're looking for findNext(), but can easily break on a findPrevious() where the result can be a perfectly valid 0.
I'd much prefer if the conditions are made explicit - if ($firstNonEmpty !== false) - to make it clearer what is actually being checked and to stabilize the code.

Good point, should be fixed now.

On that note: the check for T_OPEN_PARENTHESIS does not check against $next being falsy, while the other two findNext() calls do.
Question: are the "falsy" checks even necessary ? Keep in mind that these are function names which have already been validated by the parent::is_targetted_token() function...

I opted not to include a check for $next being false because that is already checked in the parent method. That is not the case for the other two findNext() calls, and that is why I added the checks which I believe are necessary.

On a somewhat related note, a function name followed by an opening parenthesis with nothing after it, for example __(, is considered as a function call by the abstract while I think it should not (it could be a first class callable). This was already the behavior before the changes proposed in this PR, and I decided not to change it, at least not here. I'm not sure if the current behavior is intentional or not. I added a test documeting this behavior.

I also think the tests could be improved a little more too as there are a number of things not tested now, but it could be argued that that should be left for the abstract in PHPCSUtils.

Yeah, my original idea was precisely to leave adding more tests to the abstract in PHPCSUtils. But I went ahead and added the tests that you suggested.

Live coding (those implicit conditions) x 2.

Not sure if I understand the times two part. I added the live coding tests that I think are relevant. Please let me know if I'm still missing some live coding tests.

[jrfnl]

I'd much prefer if the conditions are made explicit - if ($firstNonEmpty !== false) - to make it clearer what is actually being checked and to stabilize the code.

Good point, should be fixed now.

👍🏻

Question: are the "falsy" checks even necessary ? Keep in mind that these are function names which have already been validated by the parent::is_targetted_token() function...

I opted not to include a check for $next being false because that is already checked in the parent method. That is not the case for the other two findNext() calls, and that is why I added the checks which I believe are necessary.

👍🏻

On a somewhat related note, a function name followed by an opening parenthesis with nothing after it, for example __(, is considered as a function call by the abstract while I think it should not (it could be a first class callable). This was already the behavior before the changes proposed in this PR, and I decided not to change it, at least not here. I'm not sure if the current behavior is intentional or not. I added a test documeting this behavior.

I'd be fine with changing that behaviour and I believe this PR is the right place to do it.

To be honest, I thought the abstract already checked for open + close parenthesis, but I may have been confused with the abstract in PHPCompatibility. (👈🏻 This is why these abstracts should move to PHPCSUtils 😉 )

Do note that the process_matched_token() method calls the PassedParameters::getParameters() method, which includes a similar check via the PassedParameters::hasParameters() method, and will return an empty array for live coding/parse errors, which means that any incomplete function call would currently be relegated to the process_no_parameters() method.

Having said that, I agree with you that it would be better for the abstract to not act on unfinished function calls altogether.

Yeah, my original idea was precisely to leave adding more tests to the abstract in PHPCSUtils. But I went ahead and added the tests that you suggested.

Looks like the tests I had in mind were all added, so all good. Only thing missing is the @covers tag to indicate that the I18nTextDomainFixerUnitTest now also covers the AbstractFunctionParameterSniff abstract.

Looks like previous tests related to this abstract were added to the StrictInArrayUnitTest sniff. No need to move the new tests IMO, but the @covers tag should be added.

[rodrigoprimo]

Having said that, I agree with you that it would be better for the abstract to not act on unfinished function calls altogether.

@jrfnl, I went ahead and pushed a new commit to make the abstract ignore unfinished function calls.

Looks like the tests I had in mind were all added, so all good. Only thing missing is the @Covers tag to indicate that the I18nTextDomainFixerUnitTest now also covers the AbstractFunctionParameterSniff abstract.

Done

[jrfnl]

@rodrigoprimo Thanks for working on this. LGTM, let's get this merged!

[jrfnl]

@rodrigoprimo Would you like to clean up the commits so it's fully ready for merge ?

[rodrigoprimo]

@jrfnl, sure! I combined all the commits related to changes to ensure that the abstract class does not treat first class callable and function imports as a function call into the first commit, and left the commit to bail early if the function call is unfinished alone.

[jrfnl]

I combined all the commits related to changes to ensure that the abstract class does not treat first class callable and function imports as a function call into the first commit, and left the commit to bail early if the function call is unfinished alone.

@rodrigoprimo Excellent! Thanks.

[dingo-d]

Left just one minor question.

[rodrigoprimo]

I will create a separate issue to check if the sniffs that extend this abstract need to be updated to handle variable unpacking correctly.

@jrfnl, I just created the issue: #2521. I will wait for your feedback before creating a similar issue for PHPCompatibility.

[jrfnl]

@jrfnl, I just created the issue: #2521. I will wait for your feedback before creating a similar issue for PHPCompatibility.

@rodrigoprimo What kind of feedback are you expecting ? Or what are you expecting feedback on ?

[rodrigoprimo]

@rodrigoprimo What kind of feedback are you expecting ? Or what are you expecting feedback on ?

@jrfnl, that the issue that I created is clear and correctly describes what needs to be done.

[jrfnl]

@rodrigoprimo What kind of feedback are you expecting ? Or what are you expecting feedback on ?

@jrfnl, that the issue that I created is clear and correctly describes what needs to be done.

Ah. Well, seems clear enough to me. Could do with a code sample of the syntax the sniffs should be tested against, but other than that, all good.

[rodrigoprimo]

I just created the PHPCompatiblity issue: PHPCompatibility/PHPCompatibility#1799

Ah. Well, seems clear enough to me. Could do with a code sample of the syntax the sniffs should be tested against, but other than that, all good.

I will add a few code samples to both issues.