From cbf5d3de74bd27e4344c7fb4d4c7a2ccd5509755 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 14 Nov 2022 03:03:10 +0100 Subject: [PATCH 1/3] Add `Style/RedundantParentheses` rule --- .../rule/style/redundant_parentheses_spec.cr | 95 +++++++++++++++++++ src/ameba/rule/style/redundant_parentheses.cr | 61 ++++++++++++ 2 files changed, 156 insertions(+) create mode 100644 spec/ameba/rule/style/redundant_parentheses_spec.cr create mode 100644 src/ameba/rule/style/redundant_parentheses.cr diff --git a/spec/ameba/rule/style/redundant_parentheses_spec.cr b/spec/ameba/rule/style/redundant_parentheses_spec.cr new file mode 100644 index 00000000..2968b975 --- /dev/null +++ b/spec/ameba/rule/style/redundant_parentheses_spec.cr @@ -0,0 +1,95 @@ +require "../../../spec_helper" + +module Ameba::Rule::Style + subject = RedundantParentheses.new + + describe RedundantParentheses do + {% for keyword in %w(if unless while until) %} + context "{{ keyword.id }}" do + it "reports if redundant parentheses are found" do + source = expect_issue subject, <<-CRYSTAL, keyword: {{ keyword }} + %{keyword} (foo > 10) + _{keyword} # ^^^^^^^^^^ error: Redundant parentheses + foo + end + CRYSTAL + + expect_correction source, <<-CRYSTAL + {{ keyword.id }} foo > 10 + foo + end + CRYSTAL + end + end + {% end %} + + context "case" do + it "reports if redundant parentheses are found" do + source = expect_issue subject, <<-CRYSTAL + case (foo = @foo) + # ^^^^^^^^^^^^ error: Redundant parentheses + when String then "string" + when Symbol then "symbol" + end + CRYSTAL + + expect_correction source, <<-CRYSTAL + case foo = @foo + when String then "string" + when Symbol then "symbol" + end + CRYSTAL + end + end + + context "properties" do + context "#exclude_ternary=" do + it "skips ternary control expressions by default" do + expect_no_issues subject, <<-CRYSTAL + (foo > bar) ? true : false + CRYSTAL + end + + it "allows to configure assignments" do + rule = Rule::Style::RedundantParentheses.new + rule.exclude_ternary = false + + expect_issue rule, <<-CRYSTAL + (foo > bar) ? true : false + # ^^^^^^^^^ error: Redundant parentheses + CRYSTAL + + expect_no_issues subject, <<-CRYSTAL + (foo && bar) ? true : false + CRYSTAL + + expect_no_issues subject, <<-CRYSTAL + (foo || bar) ? true : false + CRYSTAL + end + end + + context "#exclude_assignments=" do + it "reports assignments by default" do + expect_issue subject, <<-CRYSTAL + if (foo = @foo) + # ^^^^^^^^^^^^ error: Redundant parentheses + foo + end + CRYSTAL + end + + it "allows to configure assignments" do + rule = Rule::Style::RedundantParentheses.new + rule.exclude_assignments = true + + expect_no_issues rule, <<-CRYSTAL + if (foo = @foo) + foo + end + CRYSTAL + end + end + end + end +end diff --git a/src/ameba/rule/style/redundant_parentheses.cr b/src/ameba/rule/style/redundant_parentheses.cr new file mode 100644 index 00000000..15c5bf20 --- /dev/null +++ b/src/ameba/rule/style/redundant_parentheses.cr @@ -0,0 +1,61 @@ +module Ameba::Rule::Style + # A rule that disallows redundant parentheses around control expressions. + # + # For example, this is considered invalid: + # + # ``` + # if (foo == 42) + # do_something + # end + # ``` + # + # And should be replaced by the following: + # + # ``` + # if foo == 42 + # do_something + # end + # ``` + # + # YAML configuration example: + # + # ``` + # Style/RedundantParentheses: + # Enabled: true + # ExcludeTernary: true + # ExcludeAssignments: false + # ``` + class RedundantParentheses < Base + properties do + description "Disallows redundant parentheses around control expressions" + + exclude_ternary true + exclude_assignments false + end + + MSG = "Redundant parentheses" + + def test(source, node : Crystal::If | Crystal::Unless | Crystal::Case | Crystal::While | Crystal::Until) + is_ternary = node.is_a?(Crystal::If) && node.ternary? + + return if exclude_ternary && is_ternary + + return unless (cond = node.cond).is_a?(Crystal::Expressions) + return unless cond.keyword.paren? + + return unless exp = cond.single_expression? + + case exp + when Crystal::BinaryOp + return if is_ternary + when Crystal::Assign, Crystal::OpAssign + return if exclude_assignments + end + + issue_for cond, MSG do |corrector| + corrector.remove_trailing(cond, 1) + corrector.remove_leading(cond, 1) + end + end + end +end From 3340613a6a3a50b03b6bc4cbbdc53139af37b57b Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Mon, 14 Nov 2022 17:35:07 +0100 Subject: [PATCH 2/3] Fix newly found issue --- src/ameba/spec/annotated_source.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/spec/annotated_source.cr b/src/ameba/spec/annotated_source.cr index d121d560..874fd936 100644 --- a/src/ameba/spec/annotated_source.cr +++ b/src/ameba/spec/annotated_source.cr @@ -73,7 +73,7 @@ class Ameba::Spec::AnnotatedSource private def message_to_regex(expected_annotation) String.build do |io| offset = 0 - while (index = expected_annotation.index(ABBREV, offset)) + while index = expected_annotation.index(ABBREV, offset) io << Regex.escape(expected_annotation[offset...index]) io << ".*?" offset = index + ABBREV.size From 935296b0410d87a6f3b98ce714595a06cdd904e0 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Tue, 15 Nov 2022 00:46:21 +0100 Subject: [PATCH 3/3] Refactor ternary handling in `Style/RedundantParentheses` rule --- .../rule/style/redundant_parentheses_spec.cr | 16 ++++++---- src/ameba/rule/style/redundant_parentheses.cr | 29 ++++++++++++------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/spec/ameba/rule/style/redundant_parentheses_spec.cr b/spec/ameba/rule/style/redundant_parentheses_spec.cr index 2968b975..c6890332 100644 --- a/spec/ameba/rule/style/redundant_parentheses_spec.cr +++ b/spec/ameba/rule/style/redundant_parentheses_spec.cr @@ -55,16 +55,22 @@ module Ameba::Rule::Style rule.exclude_ternary = false expect_issue rule, <<-CRYSTAL - (foo > bar) ? true : false - # ^^^^^^^^^ error: Redundant parentheses + (foo.empty?) ? true : false + # ^^^^^^^^^^ error: Redundant parentheses CRYSTAL expect_no_issues subject, <<-CRYSTAL (foo && bar) ? true : false - CRYSTAL - - expect_no_issues subject, <<-CRYSTAL (foo || bar) ? true : false + (foo = @foo) ? true : false + foo == 42 ? true : false + (foo = 42) ? true : false + (foo > 42) ? true : false + (foo >= 42) ? true : false + (3 >= foo >= 42) ? true : false + (3.in? 0..42) ? true : false + (yield 42) ? true : false + (foo rescue 42) ? true : false CRYSTAL end end diff --git a/src/ameba/rule/style/redundant_parentheses.cr b/src/ameba/rule/style/redundant_parentheses.cr index 15c5bf20..b0c1ec15 100644 --- a/src/ameba/rule/style/redundant_parentheses.cr +++ b/src/ameba/rule/style/redundant_parentheses.cr @@ -22,35 +22,44 @@ module Ameba::Rule::Style # ``` # Style/RedundantParentheses: # Enabled: true - # ExcludeTernary: true + # ExcludeTernary: false # ExcludeAssignments: false # ``` class RedundantParentheses < Base properties do description "Disallows redundant parentheses around control expressions" - exclude_ternary true + exclude_ternary false exclude_assignments false end MSG = "Redundant parentheses" + protected def strip_parentheses?(node, in_ternary) : Bool + case node + when Crystal::BinaryOp, Crystal::ExceptionHandler + !in_ternary + when Crystal::Call + !in_ternary || node.has_parentheses? || node.args.empty? + when Crystal::Yield + !in_ternary || node.has_parentheses? || node.exps.empty? + when Crystal::Assign, Crystal::OpAssign, Crystal::MultiAssign + !in_ternary && !exclude_assignments + else + true + end + end + def test(source, node : Crystal::If | Crystal::Unless | Crystal::Case | Crystal::While | Crystal::Until) is_ternary = node.is_a?(Crystal::If) && node.ternary? - return if exclude_ternary && is_ternary + return if is_ternary && exclude_ternary return unless (cond = node.cond).is_a?(Crystal::Expressions) return unless cond.keyword.paren? return unless exp = cond.single_expression? - - case exp - when Crystal::BinaryOp - return if is_ternary - when Crystal::Assign, Crystal::OpAssign - return if exclude_assignments - end + return unless strip_parentheses?(exp, is_ternary) issue_for cond, MSG do |corrector| corrector.remove_trailing(cond, 1)