diff --git a/crates/ast/src/ast/modifier.rs b/crates/ast/src/ast/modifier.rs index 0336c56..dba58aa 100644 --- a/crates/ast/src/ast/modifier.rs +++ b/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,12 @@ impl Sequence { 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 +159,9 @@ impl Sequence { } 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 +203,20 @@ impl Sequence { 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(..))) + } } diff --git a/crates/ast/src/node.rs b/crates/ast/src/node.rs index cdf779b..7fcd99f 100644 --- a/crates/ast/src/node.rs +++ b/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)]; diff --git a/crates/formatter/src/format/mod.rs b/crates/formatter/src/format/mod.rs index 477e073..38afaa2 100644 --- a/crates/formatter/src/format/mod.rs +++ b/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), } }) } diff --git a/crates/lexer/src/lib.rs b/crates/lexer/src/lib.rs index 6e2f6aa..4dfe432 100644 --- a/crates/lexer/src/lib.rs +++ b/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; } diff --git a/crates/linter/src/plugin/redundancy/mod.rs b/crates/linter/src/plugin/redundancy/mod.rs index 2f71870..92688ef 100644 --- a/crates/linter/src/plugin/redundancy/mod.rs +++ b/crates/linter/src/plugin/redundancy/mod.rs @@ -1,3 +1,5 @@ +use rules::redundant_write_visibility::RedundantWriteVisibilityRule; + 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(RedundantWriteVisibilityRule), ] } } diff --git a/crates/linter/src/plugin/redundancy/rules/mod.rs b/crates/linter/src/plugin/redundancy/rules/mod.rs index e5caa37..b068f21 100644 --- a/crates/linter/src/plugin/redundancy/rules/mod.rs +++ b/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; diff --git a/crates/linter/src/plugin/redundancy/rules/redundant_write_visibility.rs b/crates/linter/src/plugin/redundancy/rules/redundant_write_visibility.rs new file mode 100644 index 0000000..0685aca --- /dev/null +++ b/crates/linter/src/plugin/redundancy/rules/redundant_write_visibility.rs @@ -0,0 +1,57 @@ +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 RedundantWriteVisibilityRule; + +impl Rule for RedundantWriteVisibilityRule { + fn get_name(&self) -> &'static str { + "redundant-write-visibility" + } + + fn get_default_level(&self) -> Option { + Some(Level::Help) + } +} + +impl<'a> Walker> for RedundantWriteVisibilityRule { + 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::primary(write_visibility.span()).with_message("redundant write visibility."), + ]); + + context.report_with_fix(issue, |plan| { + let range = write_visibility.span().to_range(); + + plan.delete(range, mago_fixer::SafetyClassification::PotentiallyUnsafe) + }); + } + _ => {} + } + } +} diff --git a/crates/parser/src/internal/modifier.rs b/crates/parser/src/internal/modifier.rs index 6827b4a..4462f25 100644 --- a/crates/parser/src/internal/modifier.rs +++ b/crates/parser/src/internal/modifier.rs @@ -36,6 +36,8 @@ pub fn parse_optional_modifier(stream: &mut TokenStream<'_, '_>) -> Result 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), })) } diff --git a/crates/semantics/src/walker.rs b/crates/semantics/src/walker.rs index 1335d87..be32299 100644 --- a/crates/semantics/src/walker.rs +++ b/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,22 +1038,21 @@ 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()) - .with_annotation( - Annotation::primary(modifier.span()).with_message("`private(set)` modifier"), - ) - .with_annotation( - Annotation::secondary(method.span()).with_message(format!( - "method `{}::{}` defined here", - class_like_name, method_name, - )), - ) - .with_annotation( - Annotation::secondary(class_like_span) - .with_message(format!("{} `{}` is defined here", class_like_kind, class_like_fqcn)), - ), + 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")) + .with_annotation( + Annotation::secondary(method.span()) + .with_message(format!("method `{}::{}` defined here", class_like_name, method_name,)), + ) + .with_annotation( + Annotation::secondary(class_like_span) + .with_message(format!("{} `{}` is defined here", class_like_kind, class_like_fqcn)), + ), ); } } @@ -1591,7 +1590,12 @@ impl SemanticsWalker { let mut last_visibility: Option = 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 +2071,7 @@ impl Walker> 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 +2790,8 @@ impl Walker> for SemanticsWalker { Modifier::Static(_) | Modifier::Abstract(_) | Modifier::PrivateSet(_) + | Modifier::ProtectedSet(_) + | Modifier::PublicSet(_) | Modifier::Public(_) | Modifier::Protected(_) | Modifier::Private(_) => { @@ -3297,7 +3303,7 @@ impl Walker> 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!( diff --git a/crates/token/src/lib.rs b/crates/token/src/lib.rs index a88ba59..a45695a 100644 --- a/crates/token/src/lib.rs +++ b/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,23 @@ 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 +548,9 @@ impl TokenKind { | "private" | "private(set)" | "protected" + | "protected(set)" | "public" + | "public(set)" | "include" | "include_once" | "eval" @@ -1158,9 +1171,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 };