SONARPHP-1584 Support Asymmetric Property Visibility (#1335)

Author: rudy-regazzoni-sonarsource

php-frontend/src/main/java/org/sonar/php/parser/LexicalConstant.java

@@ -142,6 +142,11 @@ public class LexicalConstant {
    */
   public static final String NUMERIC_LITERAL = EXPONENT_DNUM + "|" + DNUM + "|" + INTEGER_LITERAL;
 
+  /**
+   * Other
+   */
+  public static final String ASYMMETRIC_VISIBILITY_MODIFIER = "(?i)(?:public|protected|private)\\(set\\)";
+
   private LexicalConstant() {
   }
 

php-frontend/src/main/java/org/sonar/php/parser/PHPGrammar.java

@@ -440,6 +440,7 @@ public SyntaxToken VISIBILITY_MODIFIER() {
   public SyntaxToken MEMBER_MODIFIER() {
     return b.<SyntaxToken>nonterminal(PHPLexicalGrammar.MEMBER_MODIFIER).is(
       b.firstOf(
+        b.token(PHPLexicalGrammar.ASYMMETRIC_VISIBILITY_MODIFIER),
         VISIBILITY_MODIFIER(),
         b.token(PHPKeyword.READONLY),
         b.token(PHPKeyword.STATIC),
@@ -500,7 +501,7 @@ public ParameterTree PARAMETER() {
       f.parameter(
         b.zeroOrMore(ATTRIBUTE_GROUP()),
         b.zeroOrMore(
-          b.firstOf(VISIBILITY_MODIFIER(), b.token(PHPKeyword.READONLY))),
+          b.firstOf(b.token(PHPLexicalGrammar.ASYMMETRIC_VISIBILITY_MODIFIER), VISIBILITY_MODIFIER(), b.token(PHPKeyword.READONLY))),
         b.optional(DECLARED_TYPE()),
         b.optional(b.token(PHPPunctuator.AMPERSAND)),
         b.optional(b.token(PHPPunctuator.ELLIPSIS)),

php-frontend/src/main/java/org/sonar/php/parser/PHPLexicalGrammar.java

@@ -57,6 +57,7 @@ public enum PHPLexicalGrammar implements GrammarRuleKey {
   MEMBER_MODIFIER,
   CLASS_CONST_MODIFIER,
   VISIBILITY_MODIFIER,
+  ASYMMETRIC_VISIBILITY_MODIFIER,
   MEMBER_CONST_DECLARATION,
   FUNCTION_CALL_ARGUMENT,
 
@@ -312,6 +313,7 @@ public static void lexical(LexerlessGrammarBuilder b) {
     b.rule(BACKTICK).is("`");
     // FIXME: this recovery is introduce in order to parse ${var}, as expression cannot match keywords.
     b.rule(SEMI_COMPLEX_RECOVERY_EXPRESSION).is(b.regexp("[^}]++"));
+    b.rule(ASYMMETRIC_VISIBILITY_MODIFIER).is(SPACING, b.regexp(LexicalConstant.ASYMMETRIC_VISIBILITY_MODIFIER));
 
     // Identifier
     b.rule(WHITESPACES).is(b.regexp("[" + LexicalConstant.WHITESPACE + "]*+"));

php-frontend/src/test/java/org/sonar/php/parser/PHPParserTest.java

@@ -23,10 +23,12 @@
 import org.sonar.plugins.php.api.tree.expression.BinaryExpressionTree;
 import org.sonar.plugins.php.api.tree.statement.ForEachStatementTree;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
 class PHPParserTest {
 
   @Test
-  void expression() {
+  void shouldParseExpression() {
     ActionParser<Tree> parser = PHPParserBuilder.createParser(PHPLexicalGrammar.EXPRESSION);
     BinaryExpressionTree tree = (BinaryExpressionTree) parser.parse("$a + $b + $c");
     Assertions.assertThat(tree.getParent()).isNull();
@@ -39,10 +41,24 @@ void expression() {
   }
 
   @Test
-  void forEach() {
+  void shouldParseForEach() {
     ActionParser<Tree> parser = PHPParserBuilder.createParser(PHPLexicalGrammar.FOREACH_STATEMENT);
     ForEachStatementTree tree = (ForEachStatementTree) parser.parse("foreach ($arr as &$value) { }");
     Assertions.assertThat(tree.expression().getParent()).isSameAs(tree);
   }
 
+  @Test
+  void shouldParseAsymmetricVisibilityInConstructor() {
+    ActionParser<Tree> parser = PHPParserBuilder.createParser(PHPLexicalGrammar.COMPILATION_UNIT);
+    assertDoesNotThrow(() -> parser.parse("""
+      <?php
+      class Book
+      {
+        public function __construct(
+          public private(set) string $title,
+              public protected(set) string $author,
+              protected private(set) int $pubYear,
+          ) {}
+      }"""));
+  }
 }

php-frontend/src/test/java/org/sonar/php/parser/declaration/ClassMemberTest.java

@@ -30,6 +30,12 @@ void test() {
       .matches("const A;")
       .matches("private function f() {}")
       .matches("public readonly string $prop;")
-      .matches("public int|null|(A&B) $a;");
+      .matches("public int|null|(A&B) $a;")
+      .matches("public(set) string $a;")
+      .matches("protected(set) string $a;")
+      .matches("private(set) string $a;")
+      .notMatches("public( set) string $a;") // spaces are not accepted
+      .notMatches("public(set ) string $a;")
+      .notMatches("public( set ) string $a;");
   }
 }

php-frontend/src/test/java/org/sonar/php/parser/declaration/MethodDeclarationTest.java

@@ -53,6 +53,8 @@ void construct() {
       .matches("public function __construct(protected readonly $prop) {}")
       .matches("public function __construct(protected readonly string $prop) {}")
       .matches("public function __construct(readonly protected $prop) {}")
+      .matches("public function __construct(readonly public protected(set) $prop) {}")
+      .matches("public function __construct(protected(set) $prop) {}")
       .matches("public function __construct(readonly protected string $prop) {}")
       .matches("public function __construct(readonly $prop) {}")
       .matches("public function __construct(public $p { get; }) {}")

php-frontend/src/test/java/org/sonar/php/tree/impl/declaration/ClassPropertyDeclarationTreeTest.java

@@ -55,15 +55,31 @@ void shouldParseVariable() {
       .matches("readonly $a;")
       .matches("var $a;")
 
+      // all possible asymmetric visibility
+      .matches("public(set) string $a;")
+      .matches("protected(set) string $a;")
+      .matches("private(set) string $a;")
+
+      // any order is possible for modifiers, including asymmetric visibility
+      .matches("readonly public protected(set) final string $prop;")
+      .matches("public readonly protected(set) final string $prop;")
+      .matches("readonly protected(set) public final string $prop;")
+      .matches("final readonly public protected(set) string $prop;")
+      .matches("readonly public final protected(set) string $prop;")
+
+      // not valid php, but we still parse these
       .matches("final $a;")
       .matches("public private $a;")
+      .matches("private public(set) string $a;")
 
       .matches("public string $a { get; }")
       .matches("public string $a { get { return $this-> a + 1; } }")
       .matches("public string $a { final set($value) => $value - 1; }")
+      .matches("public protected(set) string $a { get; }")
 
       .notMatches("public final A;")
-      .notMatches("$a;");
+      .notMatches("$a;")
+      .notMatches("public( set ) string $a;");
   }
 
   @Test
@@ -255,6 +271,14 @@ void shouldSupportPropertyHook() {
     assertThat(tree.eosToken()).isNull();
   }
 
+  @Test
+  void shouldSupportAsymmetricVisibilityModifier() {
+    ClassPropertyDeclarationTree tree = parse("protected(set) string $staticProp;", PHPLexicalGrammar.CLASS_VARIABLE_DECLARATION);
+    assertThat(tree.is(Kind.CLASS_PROPERTY_DECLARATION)).isTrue();
+    assertThat(tree.modifierTokens()).extracting(SyntaxToken::text).containsExactly("protected(set)");
+    assertThat(builtinType(tree)).isEqualTo("string");
+  }
+
   /**
    * Get list of all modifier token texts
    */

php-frontend/src/test/java/org/sonar/php/tree/symbols/SymbolTableImplTest.java

@@ -130,6 +130,7 @@ void testClassFields() {
 
     assertThat(field.name()).isEqualTo(fieldName);
     assertThat(field.hasModifier("public")).isTrue();
+    assertThat(field.hasModifier("private(set)")).isTrue();
     assertThat(field.is(Symbol.Kind.FIELD)).isTrue();
 
     assertThat(constantField.name()).isEqualTo(constantName);

php-frontend/src/test/resources/symbols/symbolTable.php

@@ -17,7 +17,7 @@ function f() {                                // f
 }
 
 class A {                                     // A
-  public $fieldOne, $field2 = 1;                // $field1, $field2
+  public private(set) string $fieldOne, $field2 = 1;                // $field1, $field2
   const CONSTANT_FIELD;                       // $constantField
 
   public function f($p = 12) {                     // f, $p