Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 3.7.2

Bug #18152 While and do-while with AbstractPatternSniff
Submitted: 2010-12-28 14:18 UTC
From: ryba Assigned: squiz
Status: Closed Package: PHP_CodeSniffer (version 1.3.0RC1)
PHP Version: 5.3.3 OS: Ubuntu Linux 10.10
Roadmaps: (Not assigned)    
Subscription  
Comments Add Comment Add patch


Anyone can comment on a bug. Have a simpler test case? Does it work for you on a different platform? Let us know! Just going to say 'Me too!'? Don't clutter the database with that please !
Your email address:
MUST BE VALID
Solve the problem : 30 + 18 = ?

 
 [2010-12-28 14:18 UTC] ryba (Piotr Karas)
Description: ------------ First of all, I added some var dumps in AbstactPatternSniff class in process method, something like this: // line 235 $errors = $this->processPattern($patternInfo, $phpcsFile, $stackPtr); if ($errors === false) { // The pattern didn't match. var_dump( 'DID NOT MATCH (line:' . $tokens[$stackPtr]['line'] . '): ' . $patternInfo['pattern_code'] ); continue; } else if (empty($errors) === true) { // The pattern matched, but there were no errors. var_dump( 'MATCHED, NO ERRORS (line:' . $tokens[$stackPtr]['line'] . '): ' . $patternInfo['pattern_code'] ); break; } So that I can distinguish between "no error at all" and "no error because of possible match problem". The problems are: 1) I would like to have a working pattern like this: 'doEOL...{EOL...}EOL...while (...);EOL'. Unfortunately, it won't match (sample code #1). The closest I ever got to get it matching is: 'doEOL...while (...);EOL'. Why? 2) I have two patterns that have "while" keyword in them: - A: 'while (...)EOL...{EOL' - B: 'doEOL...while (...);EOL' For some reason, when I test sample code #1, the sniff attempts to test it with both A and B, and my dump says: --- string(52) "MATCHED, NO ERRORS (line:2): doEOL...while (...);EOL" string(45) "DID NOT MATCH (line:6): while (...)EOL...{EOL" --- The good thing is that A is not matched, good enough for me. However, when I move to sample code #2, which only ads some extra stuff after the control structure, I get this: --- string(52) "MATCHED, NO ERRORS (line:2): doEOL...while (...);EOL" string(48) "MATCHED, NO ERRORS (line:8): for (...)EOL...{EOL" --- BUT THEN I GET an error because A was not satisfied: 6 | ERROR | Expected "while (...)\n...{\n"; found "while (...)...{\n" | | (mSCollection.ControlStructures.ControlSignature) So my questions here are: - Why would A that has a curly bracket in it be matched against a do-while control structure? - Why does the behavior change from sample code #1 to #2? Sorry if I'm getting things wrong here, spent couple of hours experimenting and got me nowhere... Test script: --------------- Sample code #1: <?php do { $x++; } while ( $x < 10 ); ------- Sample code #2: <?php do { $x++; } while ( $x < 10 ); for ( $s = 0; $s <= 123; $s++ ) { $s--; } -------

Comments

 [2011-01-12 06:40 UTC] squiz (Greg Sherwood)
I've committed a change to AbstactPatternSniff in SVN that might fix your issue (especially for part #1). If was not creating the pattern correctly in some cases. Can you please update from SVN and try again to see if this fixes your sniff. To replace your existing phpcs install with one from SVN: pear uninstall PHP_CodeSniffer svn co http://svn.php.net/repository/pear/packages/PHP_CodeSniffer/trunk PHP_CodeSniffer cd PHP_CodeSniffer pear install -f package.xml
 [2011-01-12 06:40 UTC] squiz (Greg Sherwood)
-Status: Open +Status: Feedback -Assigned To: +Assigned To: squiz
 [2011-01-12 13:03 UTC] ryba (Piotr Karas)
Yes, part #1 seems to be solved, I was able to match the entire do-while control structre with just one pattern and sniff a variety of modifications, all matched and reported, so I think we're good. Part #2 is still a problem.
 [2011-01-12 14:00 UTC] squiz (Greg Sherwood)
Thanks for checking. I'll look into part #2 some more.
 [2011-01-13 05:18 UTC] squiz (Greg Sherwood)
The problem you are having with part #2 is that your patterns are not specific enough. The reason WHILE in DO WHILE is matching your normal WHILE pattern is because of the ... in the pattern: while (...)EOL...{EOL You are saying that after the newline, there can be any number of characters and then an opening brace. In your second sample code, there is an opening brace on line 9 (from the FOR) so the pattern matcher skips ahead to there and keeps checking. The skip part of the pattern will skip anything. There is no way you can tell it to stay on a single line or to stop when it hits specific tokens. I'm hoping I'll be able to add a few more checks into the skip pattern so it can figure out that something like a brace does not belong to the code it started checking. Will keep you posted.
 [2011-01-13 05:44 UTC] squiz (Greg Sherwood)
I've committed another change to try and make the pattern matcher smarter. It appears to be working with your two patterns now. To test: svn up pear install -f package.xml There were no removed files so no need to uninstall first.
 [2011-01-13 12:35 UTC] ryba (Piotr Karas)
1) Please give me some time to test the new version - I'll do that later today. 2) As to my patterns being not specific enough, here's my complete pattern list: 'while (...)EOL...{EOL', 'foreach (...)EOL...{EOL', 'for (...)EOL...{EOL', 'if (...)EOL...{EOL', 'switch (...)EOL...{EOL', 'EOL...else if (...)EOL...{EOL', 'EOL...elseif (...)EOL...{EOL', 'EOL...elseEOL...{EOL', 'tryEOL...{EOL', 'catch (...)EOL...{EOL', 'doEOL...{EOL...}EOL...while (...);EOL' Now, I didn't know which other construct to use other than ... to represent the fact that there can be some indentation (for example, in "for (...)EOL...{EOL", what I wanted to achieve is that: a) there's a space between for and the condition parenthesis b) the opening curly bracket is somewhere in the line below the first line Since this for can be indented (as in a class method), I thought placing ... was necessary. Is there another construct to represent whitespace?
 [2011-01-13 12:53 UTC] squiz (Greg Sherwood)
Sorry. I meant your patterns are not specific enough for the AbstractPatternSniff. They are fine, but it needs more real tokens and less skip tokens (...). But I've made changes to hopefully fix this and allow ... patterns to not confuse it.
 [2011-01-13 19:47 UTC] ryba (Piotr Karas)
OK, part #2 seems to be taken care of. Your latest update solved the problem as well as prevented do-while control structures from being checked with standard while pattern, which means all my tests are now 100% satisfied with the patterns I mentioned. And by the way, none of other control structures pattern tests changed in any way in my tests, so seems like we're good. Thanks!
 [2011-01-14 04:17 UTC] squiz (Greg Sherwood)
-Status: Feedback +Status: Closed
Good to hear, and thanks a lot for all your testing.