Various: Add support for protected(set) and public(set), add rule to …

…check for redundant vis

crates/ast/src/ast/modifier.rs

@@ -24,7 +24,9 @@ pub enum Modifier {
     Abstract(Keyword),
     Readonly(Keyword),
     Public(Keyword),
+    PublicSet(Keyword),
     Protected(Keyword),
+    ProtectedSet(Keyword),
     Private(Keyword),
     PrivateSet(Keyword),
 }
@@ -37,7 +39,9 @@ impl Modifier {
             Modifier::Abstract(k) => k,
             Modifier::Readonly(k) => k,
             Modifier::Public(k) => k,
+            Modifier::PublicSet(k) => k,
             Modifier::Protected(k) => k,
+            Modifier::ProtectedSet(k) => k,
             Modifier::Private(k) => k,
             Modifier::PrivateSet(k) => k,
         }
@@ -58,7 +62,7 @@ impl Modifier {
 
     /// Returns `true` if the modifier is a write visibility modifier.
     pub fn is_write_visibility(&self) -> bool {
-        matches!(self, Modifier::PrivateSet(..))
+        matches!(self, Modifier::PrivateSet(..) | Modifier::ProtectedSet(..) | Modifier::PublicSet(..))
     }
 
     pub fn as_str<'a>(&self, interner: &'a ThreadedInterner) -> &'a str {
@@ -71,6 +75,8 @@ impl Modifier {
             Modifier::Protected(k) => interner.lookup(&k.value),
             Modifier::Private(k) => interner.lookup(&k.value),
             Modifier::PrivateSet(k) => interner.lookup(&k.value),
+            Modifier::ProtectedSet(k) => interner.lookup(&k.value),
+            Modifier::PublicSet(k) => interner.lookup(&k.value),
         }
     }
 }
@@ -85,7 +91,9 @@ impl HasSpan for Modifier {
             | Modifier::Public(value)
             | Modifier::Protected(value)
             | Modifier::Private(value)
-            | Modifier::PrivateSet(value) => value.span(),
+            | Modifier::PrivateSet(value)
+            | Modifier::ProtectedSet(value)
+            | Modifier::PublicSet(value) => value.span(),
         }
     }
 }
@@ -135,7 +143,7 @@ impl Sequence<Modifier> {
         self.iter().find(|modifier| {
             matches!(
                 modifier,
-                Modifier::Public(..) | Modifier::Protected(..) | Modifier::Private(..) | Modifier::PrivateSet(..)
+                Modifier::Public(..) | Modifier::Protected(..) | Modifier::Private(..) | Modifier::PrivateSet(..) | Modifier::ProtectedSet(..) | Modifier::PublicSet(..)
             )
         })
     }
@@ -146,7 +154,7 @@ impl Sequence<Modifier> {
     }
 
     pub fn get_first_write_visibility(&self) -> Option<&Modifier> {
-        self.iter().find(|modifier| matches!(modifier, Modifier::PrivateSet(..)))
+        self.iter().find(|modifier| matches!(modifier, Modifier::PrivateSet(..) | Modifier::ProtectedSet(..) | Modifier::PublicSet(..)))
     }
 
     /// Returns `true` if the sequence contains a visibility modifier for reading or writing.
@@ -188,4 +196,20 @@ impl Sequence<Modifier> {
     pub fn contains_private_set(&self) -> bool {
         self.iter().any(|modifier| matches!(modifier, Modifier::PrivateSet(..)))
     }
+
+    pub fn get_protected_set(&self) -> Option<&Modifier> {
+        self.iter().find(|modifier| matches!(modifier, Modifier::ProtectedSet(..)))
+    }
+
+    pub fn contains_protected_set(&self) -> bool {
+        self.iter().any(|modifier| matches!(modifier, Modifier::ProtectedSet(..)))
+    }
+
+    pub fn get_public_set(&self) -> Option<&Modifier> {
+        self.iter().find(|modifier| matches!(modifier, Modifier::PublicSet(..)))
+    }
+
+    pub fn contains_public_set(&self) -> bool {
+        self.iter().any(|modifier| matches!(modifier, Modifier::PublicSet(..)))
+    }
 }

crates/ast/src/node.rs

@@ -1608,6 +1608,8 @@ impl<'a> Node<'a> {
                 Modifier::Static(node) => Node::Keyword(node),
                 Modifier::Readonly(node) => Node::Keyword(node),
                 Modifier::PrivateSet(node) => Node::Keyword(node),
+                Modifier::ProtectedSet(node) => Node::Keyword(node),
+                Modifier::PublicSet(node) => Node::Keyword(node),
             }],
             Node::Namespace(node) => {
                 let mut children = vec![Node::Keyword(&node.r#namespace)];

crates/formatter/src/format/mod.rs

@@ -1384,6 +1384,8 @@ impl<'a> Format<'a> for Modifier {
                 Modifier::Protected(keyword) => keyword.format(f),
                 Modifier::Private(keyword) => keyword.format(f),
                 Modifier::PrivateSet(keyword) => keyword.format(f),
+                Modifier::ProtectedSet(keyword) => keyword.format(f),
+                Modifier::PublicSet(keyword) => keyword.format(f),
             }
         })
     }

crates/lexer/src/lib.rs

@@ -419,6 +419,22 @@ impl<'a, 'i> Lexer<'a, 'i> {
 
                                     break;
                                 }
+                                // special case for `public(set)`
+                                [b'(', ..] if length == 6 => {
+                                    if self.input.is_at(b"public(set)", true) {
+                                        break 'identifier (TokenKind::PublicSet, 6 + 5);
+                                    }
+
+                                    break;
+                                }
+                                // special case for `protected(set)`
+                                [b'(', ..] if length == 9 => {
+                                    if self.input.is_at(b"protected(set)", true) {
+                                        break 'identifier (TokenKind::ProtectedSet, 9 + 5);
+                                    }
+
+                                    break;
+                                }
                                 _ => {
                                     break;
                                 }

crates/linter/src/plugin/redundancy/mod.rs

@@ -1,3 +1,5 @@
+use rules::redundant_write_visibility::RedundantWriteVisibility;
+
 use crate::plugin::redundancy::rules::redundant_block::RedundantBlockRule;
 use crate::plugin::redundancy::rules::redundant_closing_tag::RedudnantClosingTagRule;
 use crate::plugin::redundancy::rules::redundant_continue::RedundantContinueRule;
@@ -37,6 +39,7 @@ impl Plugin for RedundancyPlugin {
             Box::new(RedundantFinalMethodModifierRule),
             Box::new(RedundantLabelRule),
             Box::new(RedundantIfStatementRule),
+            Box::new(RedundantWriteVisibility),
         ]
     }
 }

crates/linter/src/plugin/redundancy/rules/mod.rs

@@ -8,3 +8,4 @@ pub mod redundant_method_override;
 pub mod redundant_noop;
 pub mod redundant_parentheses;
 pub mod redundant_string_concat;
+pub mod redundant_write_visibility;

crates/linter/src/plugin/redundancy/rules/redundant_write_visibility.rs

@@ -0,0 +1,55 @@
+use mago_ast::{Modifier, Property};
+use mago_reporting::{Annotation, Issue, Level};
+use mago_span::HasSpan;
+use mago_walker::Walker;
+
+use crate::{context::LintContext, rule::Rule};
+
+#[derive(Clone, Debug)]
+pub struct RedundantWriteVisibility;
+
+impl Rule for RedundantWriteVisibility {
+    fn get_name(&self) -> &'static str {
+        "redundant-write-visibility"
+    }
+
+    fn get_default_level(&self) -> Option<mago_reporting::Level> {
+        Some(Level::Help)
+    }
+}
+
+impl<'a> Walker<LintContext<'a>> for RedundantWriteVisibility {
+    fn walk_in_property(&self, property: &Property, context: &mut LintContext<'a>) {
+        let modifiers = property.modifiers();
+
+        if modifiers.is_empty() {
+            return;
+        }
+
+        let Some(write_visibility) = modifiers.get_first_write_visibility() else {
+            return;
+        };
+
+        let Some(read_visibility) = modifiers.get_first_read_visibility() else {
+            return;
+        };
+
+        match (read_visibility, write_visibility) {
+            (Modifier::Public(_), Modifier::PublicSet(_)) | (Modifier::Protected(_), Modifier::ProtectedSet(_)) | (Modifier::Private(_), Modifier::PrivateSet(_)) => {
+                let issue = Issue::new(context.level(), "identical write visibility has no effect")
+                    .with_help("remove the redundant write visibility modifier")
+                    .with_annotations(vec![
+                        Annotation::secondary(read_visibility.span()),
+                        Annotation::secondary(write_visibility.span()),
+                    ]);
+
+                context.report_with_fix(issue, |plan| {
+                    let range = write_visibility.span().to_range();
+
+                    plan.delete(range, mago_fixer::SafetyClassification::PotentiallyUnsafe)
+                });
+            },
+            _ => {}
+        }
+    }
+}

crates/parser/src/internal/modifier.rs

@@ -36,6 +36,8 @@ pub fn parse_optional_modifier(stream: &mut TokenStream<'_, '_>) -> Result<Optio
         Some(T!["abstract"]) => Modifier::Abstract(utils::expect_any_keyword(stream)?),
         Some(T!["readonly"]) => Modifier::Readonly(utils::expect_any_keyword(stream)?),
         Some(T!["private(set)"]) => Modifier::PrivateSet(utils::expect_any_keyword(stream)?),
+        Some(T!["protected(set)"]) => Modifier::ProtectedSet(utils::expect_any_keyword(stream)?),
+        Some(T!["public(set)"]) => Modifier::PublicSet(utils::expect_any_keyword(stream)?),
         _ => return Ok(None),
     }))
 }

crates/semantics/src/walker.rs

@@ -344,7 +344,7 @@ impl SemanticsWalker {
 
                     last_read_visibility = Some(modifier.span());
                 }
-                Modifier::PrivateSet(_) => {
+                Modifier::PrivateSet(_) | Modifier::ProtectedSet(_) | Modifier::PublicSet(_) => {
                     if let Some(last_visibility) = last_write_visibility {
                         context.report(
                             Issue::error(format!(
@@ -1038,9 +1038,9 @@ impl SemanticsWalker {
                         last_visibility = Some(modifier.span());
                     }
                 }
-                Modifier::PrivateSet(_) => {
+                Modifier::PrivateSet(k) | Modifier::ProtectedSet(k) | Modifier::PublicSet(k) => {
                     context.report(
-                        Issue::error("`private(set)` modifier is not allowed on methods".to_string())
+                        Issue::error(format!("`{}` modifier is not allowed on methods", context.interner.lookup(&k.value)))
                             .with_annotation(
                                 Annotation::primary(modifier.span()).with_message("`private(set)` modifier"),
                             )
@@ -1591,7 +1591,7 @@ impl SemanticsWalker {
         let mut last_visibility: Option<Span> = None;
         for modifier in class_like_constant.modifiers.iter() {
             match modifier {
-                Modifier::Readonly(k) | Modifier::Static(k) | Modifier::Abstract(k) | Modifier::PrivateSet(k) => {
+                Modifier::Readonly(k) | Modifier::Static(k) | Modifier::Abstract(k) | Modifier::PrivateSet(k) | Modifier::ProtectedSet(k) | Modifier::PublicSet(k) => {
                     context.report(
                         Issue::error(format!(
                             "`{}` modifier is not allowed on constants",
@@ -2067,7 +2067,7 @@ impl Walker<Context<'_>> for SemanticsWalker {
 
         for modifier in class.modifiers.iter() {
             match &modifier {
-                Modifier::Static(_) | Modifier::PrivateSet(_) => {
+                Modifier::Static(_) | Modifier::PrivateSet(_) | Modifier::ProtectedSet(_) | Modifier::PublicSet(_) => {
                     context.report(
                         Issue::error(format!(
                             "class `{}` cannot have `{}` modifier",
@@ -2786,6 +2786,8 @@ impl Walker<Context<'_>> for SemanticsWalker {
                 Modifier::Static(_)
                 | Modifier::Abstract(_)
                 | Modifier::PrivateSet(_)
+                | Modifier::ProtectedSet(_)
+                | Modifier::PublicSet(_)
                 | Modifier::Public(_)
                 | Modifier::Protected(_)
                 | Modifier::Private(_) => {
@@ -3297,7 +3299,7 @@ impl Walker<Context<'_>> for SemanticsWalker {
                             last_read_visibility = Some(modifier.span());
                         }
                     }
-                    Modifier::PrivateSet(_) => {
+                    Modifier::PrivateSet(_) | Modifier::ProtectedSet(_) | Modifier::PublicSet(_) => {
                         if let Some(s) = last_write_visibility {
                             context.report(
                                 Issue::error(format!(

crates/token/src/lib.rs

@@ -222,7 +222,9 @@ pub enum TokenKind {
     Private,                     // `private`
     PrivateSet,                  // `private(set)`
     Protected,                   // `protected`
+    ProtectedSet,                // `protected(set)`
     Public,                      // `public`
+    PublicSet,                   // `public(set)`
     QualifiedIdentifier,         // `Namespace\Class`
     Question,                    // `?`
     QuestionColon,               // `?:`
@@ -488,14 +490,14 @@ impl TokenKind {
 
     #[inline(always)]
     pub fn is_visibility_modifier(&self) -> bool {
-        matches!(self, T!["public" | "protected" | "private" | "private(set)"])
+        matches!(self, T!["public" | "protected" | "private" | "private(set)" | "protected(set)" | "public(set)"])
     }
 
     #[inline(always)]
     pub fn is_modifier(&self) -> bool {
         matches!(
             self,
-            T!["public" | "protected" | "private" | "private(set)" | "static" | "final" | "abstract" | "readonly"]
+            T!["public" | "protected" | "private" | "private(set)" | "protected(set)" | "public(set)" | "static" | "final" | "abstract" | "readonly"]
         )
     }
 
@@ -537,7 +539,9 @@ impl TokenKind {
                 | "private"
                 | "private(set)"
                 | "protected"
+                | "protected(set)"
                 | "public"
+                | "public(set)"
                 | "include"
                 | "include_once"
                 | "eval"
@@ -1158,9 +1162,15 @@ macro_rules! T {
     ("protected") => {
         $crate::TokenKind::Protected
     };
+    ("protected(set)") => {
+        $crate::TokenKind::ProtectedSet
+    };
     ("public") => {
         $crate::TokenKind::Public
     };
+    ("public(set)") => {
+        $crate::TokenKind::PublicSet
+    };
     ("Qualified\\Identifier") => {
         $crate::TokenKind::QualifiedIdentifier
     };