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

JS tokenizer fails to tokenize regular expression proceeded by boolean not operator #1915

Closed
GaryJones opened this issue Feb 26, 2018 · 3 comments
Milestone

Comments

@GaryJones
Copy link
Contributor

WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter (which extends Squiz_Sniffs_WhiteSpace_OperatorSpacingSniff with minimal changes) complains about the lack of whitespace in a JavaScript regex pattern.

function ga_skiplinks() {
	'use strict';
	var element = document.getElementById( location.hash.substring( 1 ) );

	if ( element ) {
		if ( ! /^(?:a|select|input|button|textarea)$/i.test( element.tagName ) ) { // L21
			element.tabIndex = -1;
		}
		element.focus();
	}
}

Result:

 21 | ERROR | [x] Expected 1 space before "^"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 21 | ERROR | [x] Expected 1 space before "?"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 21 | ERROR | [x] Expected 1 space before "|"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 21 | ERROR | [x] Expected 1 space before "|"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 21 | ERROR | [x] Expected 1 space before "|"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 21 | ERROR | [x] Expected 1 space before "|"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 21 | ERROR | [x] Expected 1 space before "/"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)

I would expect this sniff/check to ignore JS regex patterns.

@jrfnl
Copy link
Contributor

jrfnl commented Feb 26, 2018

I've had a quick look. This isn't so much an issue with the sniff, as it is an issue with the JS tokenizer.

if ( /^(?:a|select|input|button|textarea)$/i.test( element.tagName ) ) {}

is tokenized (correctly) as - take note of token 6/T_REGULAR_EXPRESSION:

2 :: L002 :: C1 :: T_IF :: (2) :: if
3 :: L002 :: C3 :: T_WHITESPACE :: (1) ::
4 :: L002 :: C4 :: T_OPEN_PARENTHESIS :: (1) :: (
5 :: L002 :: C5 :: T_WHITESPACE :: (1) ::
6 :: L002 :: C6 :: T_REGULAR_EXPRESSION :: (39) :: /^(?:a|select|input|button|textarea)$/i
7 :: L002 :: C45 :: T_OBJECT_OPERATOR :: (1) :: .
8 :: L002 :: C46 :: T_STRING :: (4) :: test
... etc ...

The tokenizer gets confused when the boolean not ! operator is used however.

if ( ! /^(?:a|select|input|button|textarea)$/i.test( element.tagName ) ) {}

is tokenized as:

23 :: L004 :: C1 :: T_IF :: (2) :: if
24 :: L004 :: C3 :: T_WHITESPACE :: (1) ::
25 :: L004 :: C4 :: T_OPEN_PARENTHESIS :: (1) :: (
26 :: L004 :: C5 :: T_WHITESPACE :: (1) ::
27 :: L004 :: C6 :: T_BOOLEAN_NOT :: (1) :: !
28 :: L004 :: C7 :: T_WHITESPACE :: (1) ::
29 :: L004 :: C8 :: T_DIVIDE :: (1) :: /
30 :: L004 :: C9 :: T_LOGICAL_XOR :: (1) :: ^
31 :: L004 :: C10 :: T_OPEN_PARENTHESIS :: (1) :: (
32 :: L004 :: C11 :: T_INLINE_THEN :: (1) :: ?
33 :: L004 :: C12 :: T_INLINE_ELSE :: (1) :: :
34 :: L004 :: C13 :: T_STRING :: (1) :: a
35 :: L004 :: C14 :: T_BITWISE_OR :: (1) :: |
36 :: L004 :: C15 :: T_STRING :: (6) :: select
37 :: L004 :: C21 :: T_BITWISE_OR :: (1) :: |
38 :: L004 :: C22 :: T_STRING :: (5) :: input
39 :: L004 :: C27 :: T_BITWISE_OR :: (1) :: |
40 :: L004 :: C28 :: T_STRING :: (6) :: button
41 :: L004 :: C34 :: T_BITWISE_OR :: (1) :: |
42 :: L004 :: C35 :: T_STRING :: (8) :: textarea
43 :: L004 :: C43 :: T_CLOSE_PARENTHESIS :: (1) :: )
44 :: L004 :: C44 :: T_STRING :: (1) :: $
45 :: L004 :: C45 :: T_DIVIDE :: (1) :: /
46 :: L004 :: C46 :: T_STRING :: (1) :: i
47 :: L004 :: C47 :: T_OBJECT_OPERATOR :: (1) :: .
48 :: L004 :: C48 :: T_STRING :: (4) :: test
... etc ...

@GaryJones
Copy link
Contributor Author

GaryJones commented Feb 26, 2018

Thanks @jrfnl - using false === works.

In which case #1170 may be related.

@gsherwood gsherwood changed the title Squiz.WhiteSpace.OperatorSpacing.NoSpaceBefore reports on JavaScript regex pattern JS tokenizer fails to tokenize regular expression proceeded by boolean not operator Feb 27, 2018
@gsherwood gsherwood added this to the 3.3.0 milestone Feb 27, 2018
gsherwood added a commit that referenced this issue Feb 27, 2018
@gsherwood
Copy link
Member

@GaryJones You were right. It was the same problem as bug #1170. I needed to add an extra operator.

Thanks for the report.

@gsherwood gsherwood removed this from Ready for Release in PHPCS v3 Development Jun 7, 2018
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