MC-19366: Fixes that agruments with directives would result in erroneous output and that arguments could be treated as fields

jean-bernard-valentaten · jean-bernard-valentaten · commit 49f0b0b09924 · 2019-09-13T13:38:10.000+02:00
diff --git a/Magento2/Sniffs/GraphQL/ValidArgumentNameSniff.php b/Magento2/Sniffs/GraphQL/ValidArgumentNameSniff.php
@@ -6,31 +6,40 @@
 
 namespace Magento2\Sniffs\GraphQL;
 
+use GraphQL\Error\SyntaxError;
+use GraphQL\Language\AST\DocumentNode;
 use PHP_CodeSniffer\Files\File;
 
 /**
  * Detects argument names that are not specified in <kbd>cameCase</kbd>.
  */
 class ValidArgumentNameSniff extends AbstractGraphQLSniff
 {
+
     /**
      * @inheritDoc
      */
     public function register()
     {
-        return [T_OPEN_PARENTHESIS];
+        return [T_VARIABLE];
     }
 
     /**
      * @inheritDoc
      */
     public function process(File $phpcsFile, $stackPtr)
     {
-        $tokens                  = $phpcsFile->getTokens();
-        $closeParenthesisPointer = $this->getCloseParenthesisPointer($stackPtr, $tokens);
+        $tokens = $phpcsFile->getTokens();
+
+        //get the pointer to the argument list opener or bail out if none was found since then the field does not have arguments
+        $openArgumentListPointer = $this->getArgumentListOpenPointer($stackPtr, $tokens);
+        if ($openArgumentListPointer === false) {
+            return;
+        }
 
-        //if we could not find the closing parenthesis pointer, we add a warning and terminate
-        if ($closeParenthesisPointer === false) {
+        //get the pointer to the argument list closer or add a warning and terminate as we have an unbalanced file
+        $closeArgumentListPointer = $this->getArgumentListClosePointer($openArgumentListPointer, $tokens);
+        if ($closeArgumentListPointer === false) {
             $error = 'Possible parse error: Missing closing parenthesis for argument list in line %d';
             $data  = [
                 $tokens[$stackPtr]['line'],
@@ -39,18 +48,15 @@ public function process(File $phpcsFile, $stackPtr)
             return;
         }
 
-        $arguments = $this->getArguments($stackPtr, $closeParenthesisPointer, $tokens);
+        $arguments = $this->getArguments($openArgumentListPointer, $closeArgumentListPointer, $tokens);
 
-        foreach ($arguments as $argument) {
-            $pointer = $argument[0];
-            $name    = $argument[1];
-
-            if (!$this->isCamelCase($name)) {
+        foreach ($arguments as $pointer => $argument) {
+            if (!$this->isCamelCase($argument)) {
                 $type  = 'Argument';
                 $error = '%s name "%s" is not in CamelCase format';
                 $data  = [
                     $type,
-                    $name,
+                    $argument,
                 ];
 
                 $phpcsFile->addError($error, $pointer, 'NotCamelCase', $data);
@@ -61,56 +67,131 @@ public function process(File $phpcsFile, $stackPtr)
         }
 
         //return stack pointer of closing parenthesis
-        return $closeParenthesisPointer;
+        return $closeArgumentListPointer;
     }
 
     /**
-     * Finds all argument names contained in <var>$tokens</var> in range <var>$startPointer</var> to
-     * <var>$endPointer</var>.
+     * Seeks the last token of an argument definition and returns its pointer.
      *
-     * @param int $startPointer
-     * @param int $endPointer
+     * Arguments are defined as follows:
+     * <noformat>
+     *   {ArgumentName}: {ArgumentType}[ = {DefaultValue}][{Directive}]*
+     * </noformat>
+     *
+     * @param int $argumentDefinitionStartPointer
      * @param array $tokens
-     * @return array[]
+     * @return int
      */
-    private function getArguments($startPointer, $endPointer, array $tokens)
+    private function getArgumentDefinitionEndPointer($argumentDefinitionStartPointer, array $tokens)
     {
-        $argumentTokenPointer = null;
-        $argument             = '';
-        $names                = [];
-        $skipTypes            = [T_COMMENT, T_WHITESPACE];
+        $colonPointer = $this->seekToken(T_COLON, $tokens, $argumentDefinitionStartPointer);
 
-        for ($i = $startPointer + 1; $i < $endPointer; ++$i) {
-            //skip some tokens
-            if (in_array($tokens[$i]['code'], $skipTypes)) {
-                continue;
-            }
-            $argument .= $tokens[$i]['content'];
+        //the colon is always followed by a type so we can consume the token after the colon
+        $endPointer = $colonPointer + 1;
 
-            if ($argumentTokenPointer === null) {
-                $argumentTokenPointer = $i;
-            }
+        //if argument has a default value, we advance to the default definition end
+        if ($tokens[$endPointer + 1]['code'] === T_EQUAL) {
+            $endPointer += 2;
+        }
+
+        //while next token starts a directive, we advance to the end of the directive
+        while ($tokens[$endPointer + 1]['code'] === T_DOC_COMMENT_TAG) {
+            //consume next two tokens
+            $endPointer += 2;
 
-            if (preg_match('/^.+:.+$/', $argument)) {
-                list($name, $type) = explode(':', $argument);
-                $names[]              = [$argumentTokenPointer, $name];
-                $argument             = '';
-                $argumentTokenPointer = null;
+            //if next token is an opening parenthesis, we consume everything up to the closing parenthesis
+            if ($tokens[$endPointer + 1]['code'] === T_OPEN_PARENTHESIS) {
+                $endPointer = $tokens[$endPointer + 1]['parenthesis_closer'];
             }
         }
 
-        return $names;
+        return $endPointer;
+    }
+
+    /**
+     * Returns the closing parenthesis for the token found at <var>$openParenthesisPointer</var> in <var>$tokens</var>.
+     *
+     * @param int $openParenthesisPointer
+     * @param array $tokens
+     * @return bool|int
+     */
+    private function getArgumentListClosePointer($openParenthesisPointer, array $tokens)
+    {
+        $openParenthesisToken = $tokens[$openParenthesisPointer];
+        return $openParenthesisToken['parenthesis_closer'];
     }
 
     /**
-     * Seeks the next available token of type {@link T_CLOSE_PARENTHESIS} in <var>$tokens</var> and returns its pointer.
+     * Seeks the next available {@link T_OPEN_PARENTHESIS} token that comes directly after <var>$stackPointer</var>.
+     * token.
      *
      * @param int $stackPointer
      * @param array $tokens
      * @return bool|int
      */
-    private function getCloseParenthesisPointer($stackPointer, array $tokens)
+    private function getArgumentListOpenPointer($stackPointer, array $tokens)
+    {
+        //get next open parenthesis pointer or bail out if none was found
+        $openParenthesisPointer = $this->seekToken(T_OPEN_PARENTHESIS, $tokens, $stackPointer);
+        if ($openParenthesisPointer === false) {
+            return false;
+        }
+
+        //bail out if open parenthesis does not directly come after current stack pointer
+        if ($openParenthesisPointer !== $stackPointer + 1) {
+            return false;
+        }
+
+        //we have found the appropriate opening parenthesis
+        return $openParenthesisPointer;
+    }
+
+    /**
+     * Finds all argument names contained in <var>$tokens</var> in range <var>$startPointer</var> to
+     * <var>$endPointer</var>.
+     *
+     * The returned array uses token pointers as keys and argument names as values.
+     *
+     * @param int $startPointer
+     * @param int $endPointer
+     * @param array $tokens
+     * @return array<int, string>
+     */
+    private function getArguments($startPointer, $endPointer, array $tokens)
     {
-        return $this->seekToken(T_CLOSE_PARENTHESIS, $tokens, $stackPointer);
+        $argumentTokenPointer = null;
+        $argument             = '';
+        $names                = [];
+        $skipTypes            = [T_COMMENT, T_WHITESPACE];
+
+        for ($i = $startPointer + 1; $i < $endPointer; ++$i) {
+            $tokenCode = $tokens[$i]['code'];
+
+            switch (true) {
+                case in_array($tokenCode, $skipTypes):
+                    //NOP This is a toke that we have to skip
+                    break;
+                case $tokenCode === T_COLON:
+                    //we have reached the end of the argument name, thus we store its pointer and value
+                    $names[$argumentTokenPointer] = $argument;
+
+                    //advance to end of argument definition
+                    $i = $this->getArgumentDefinitionEndPointer($argumentTokenPointer, $tokens);
+
+                    //and reset temporary variables
+                    $argument             = '';
+                    $argumentTokenPointer = null;
+                    break;
+                default:
+                    //this seems to be part of the argument name
+                    $argument .= $tokens[$i]['content'];
+
+                    if ($argumentTokenPointer === null) {
+                        $argumentTokenPointer = $i;
+                    }
+            }
+        }
+
+        return $names;
     }
 }
diff --git a/Magento2/Tests/GraphQL/ValidArgumentNameUnitTest.graphqls b/Magento2/Tests/GraphQL/ValidArgumentNameUnitTest.graphqls
@@ -29,4 +29,21 @@ interface Foo {
         invalid_argument_name_snake_case: Int
         5invalidArgumentNameStatingWithNumber: Int
     ): String
-}
+}
+
+#
+# Make sure that directives on arguments do not lead to false positives
+#
+type Foo @doc(descripton: "Foo Bar Baz") {
+    myfield (
+        # Valid arguments
+        validArgument: String = "My fair lady" @doc(
+            # A comment
+            description: "This is a valid argument, spanned over multiple lines."
+        ) @foo
+        varlidArgumentWithDefaultValue: Int = 20 @doc(description: "This is another valid argument with a default value.")
+        # Invalid argument
+        invalid_argument: String @doc(description: "This is an invalid argument."),
+        invalid_argument_with_default_value: Int = 20 @doc(description: "This is another invalid argument with a default value")
+    ): String @doc(description: "Foo Bar")
+}
diff --git a/Magento2/Tests/GraphQL/ValidArgumentNameUnitTest.php b/Magento2/Tests/GraphQL/ValidArgumentNameUnitTest.php
@@ -24,6 +24,8 @@ protected function getErrorList()
             28 => 1,
             29 => 1,
             30 => 1,
+            46 => 1,
+            47 => 1,
         ];
     }
 
diff --git a/PHP_CodeSniffer/Tokenizers/GRAPHQL.php b/PHP_CodeSniffer/Tokenizers/GRAPHQL.php
@@ -7,6 +7,7 @@
 namespace PHP_CodeSniffer\Tokenizers;
 
 use GraphQL\Language\Lexer;
+use GraphQL\Language\Parser;
 use GraphQL\Language\Source;
 use GraphQL\Language\Token;
 
@@ -81,11 +82,10 @@ protected function tokenize($string)
     {
         $this->logVerbose('*** START GRAPHQL TOKENIZING ***');
 
-        $string = str_replace($this->eolChar, "\n", $string);
-        $tokens = [];
-        $lexer  = new Lexer(
-            new Source($string)
-        );
+        $string   = str_replace($this->eolChar, "\n", $string);
+        $tokens   = [];
+        $source   = new Source($string);
+        $lexer    = new Lexer($source);
 
         do {
             $kind  = $lexer->token->kind;
@@ -178,19 +178,17 @@ private function getNewLineTokens($lineStart, $lineEnd)
      */
     private function isFieldToken($stackPointer)
     {
+        //bail out if current token is nested in a parenthesis, since fields cannot be contained in parenthesises
+        if (isset($this->tokens[$stackPointer]['nested_parenthesis'])) {
+            return false;
+        }
+
         $nextToken = $this->tokens[$stackPointer + 1];
 
-        //if next token is an opening parenthesis, we seek for the closing parenthesis
+        //if next token is an opening parenthesis, we advance to the token after the closing parenthesis
         if ($nextToken['code'] === T_OPEN_PARENTHESIS) {
-            $nextPointer = $stackPointer + 1;
-            $numTokens   = count($this->tokens);
-
-            for ($i = $nextPointer; $i < $numTokens; ++$i) {
-                if ($this->tokens[$i]['code'] === T_CLOSE_PARENTHESIS) {
-                    $nextToken = $this->tokens[$i + 1];
-                    break;
-                }
-            }
+            $nextPointer = $nextToken['parenthesis_closer'] + 1;
+            $nextToken   = $this->tokens[$nextPointer];
         }
 
         //return whether current token is a string and next token is a colon
@@ -215,7 +213,6 @@ private function logVerbose($message, $level = 1)
      */
     private function processFields()
     {
-        $processingArgumentList = false;
         $processingEntity       = false;
         $numTokens              = count($this->tokens);
         $entityTypes            = [T_CLASS, T_INTERFACE];
@@ -237,15 +234,7 @@ private function processFields()
                     //we have hit the end of an entity declaration
                     $processingEntity = false;
                     break;
-                case $tokenCode === T_OPEN_PARENTHESIS:
-                    //we have hit an argument list
-                    $processingArgumentList = true;
-                    break;
-                case $tokenCode === T_CLOSE_PARENTHESIS:
-                    //we have hit an argument list end
-                    $processingArgumentList = false;
-                    break;
-                case $processingEntity && !$processingArgumentList && $this->isFieldToken($i):
+                case $processingEntity && $this->isFieldToken($i):
                     //we have hit a field
                     $this->tokens[$i]['code'] = T_VARIABLE;
                     $this->tokens[$i]['type'] = 'T_VARIABLE';

Original file line number	Diff line number	Diff line change
`@@ -24,6 +24,8 @@ protected function getErrorList()`
`24`	`24`	`28 => 1,`
`25`	`25`	`29 => 1,`
`26`	`26`	`30 => 1,`
	`27`	`+ 46 => 1,`
	`28`	`+ 47 => 1,`
`27`	`29`	`];`
`28`	`30`	`}`
`29`	`31`