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

Incorrect detection of operator in ternary + anonymous function #1782

Closed
VasekPurchart opened this issue Dec 13, 2017 · 6 comments
Closed

Incorrect detection of operator in ternary + anonymous function #1782

VasekPurchart opened this issue Dec 13, 2017 · 6 comments

Comments

@VasekPurchart
Copy link
Contributor

I think there is a problem with detection of the : operator in ternary in combination with a function. I found this out while using Squiz.WhiteSpace.OperatorSpacing, but it can be more general.

This case throws error:

<?php

$x = $foo
	? function (): int {
		return 1;
	}
	: $bar;
 4 | ERROR | [x] Expected 1 space before ":"; 0 found (Squiz.WhiteSpace.OperatorSpacing.NoSpaceBefore)

while just inverting the condition is ok:

<?php

$x = !$foo
	? $bar
	: function (): int {
		return 1;
	};
@gsherwood
Copy link
Member

gsherwood commented Dec 14, 2017

If I put both your code examples into a file like this:

<?php

$x = $foo
    ? function (): int {
        return 1;
    }
    : $bar;

$x = !$foo
    ? $bar
    : function (): int {
        return 1;
    };

And then run PHPCS, I get errors for both colons:

$ phpcs temp.php --sniffs=Squiz.WhiteSpace.OperatorSpacing
FILE: temp.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  4 | ERROR | [x] Expected 1 space before "?"; newline found
  4 | ERROR | [x] Expected 1 space before ":"; 0 found
 10 | ERROR | [x] Expected 1 space before "?"; newline found
 11 | ERROR | [x] Expected 1 space before ":"; newline found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I'm using PHPCS version 3.2.0, but I also tried 2.9.1. What version are you using?

@jrfnl
Copy link
Contributor

jrfnl commented Dec 14, 2017

@gsherwood I think the op intended to point out that the return type specifier colon : is being seen as the inline else. I.e. the tokenizer is doing something wrong here.
In the first example, the colon after function() is tokenized as T_INLINE_ELSE instead of as T_COLON, which is, how it is tokenized in the second example.
You can see this in your example code as well, as the error for the first sample is thrown on line 4, not line 7.

The return type specifier colon is not an operator and IMO should not be covered by the Squiz.WhiteSpace.OperatorSpacing sniff.
I'd personally find it more logical to have a check for spacing around the return type specifier colon in a function declaration sniff.

Fixing the tokenizing issue, should get rid of the incorrect sniff result the op reported.

@VasekPurchart
Copy link
Contributor Author

Sorry, was in a a hurry and forgot to mention the version, I tested it both on 3.1.1 and 3.2.0.

Yes I think it is excatly as @jrfnl writes, the problem seems to be in tokenization rather than the sniff, I showed the two examples because it further suggests that the tokenization is broken only when the anon function is in the true part of the ternary, suggesting the detection of the : for the ternary is thrown off by the return type of the anon fuction.

You can filter out the unrelated errors with (I have this in the ruleset, sorry for not including it):

	<rule ref="Squiz.WhiteSpace.OperatorSpacing">
		<properties>
			<property name="ignoreNewlines" value="true"/>
		</properties>
	</rule>

@gsherwood
Copy link
Member

Sorry, I missed the tokenizing issue. Makes sense now. Thanks.

@gsherwood
Copy link
Member

I've fixed up the tokenizer, although it was a bit painful. I can't think of a better way of detecting the colon though, given PHP doesn't assign them different tokens.

Thanks for reporting this issue.

@VasekPurchart
Copy link
Contributor Author

Thank you! :)

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

3 participants