Auto-fix: Add support for 'use_memberships'

HugoGGuerrier · HugoGGuerrier · commit 83e872df329d · 2024-12-10T16:55:33.000+01:00
Also enhance the rule detection.
diff --git a/lkql_checker/share/lkql/use_memberships.lkql b/lkql_checker/share/lkql/use_memberships.lkql
@@ -1,16 +1,31 @@
 import stdlib
 
+fun get_first_id(node) =
+    |" Get the first Identifier in the given ``node`` which is, either the
+    |" left part of a RelationOp, or the expression if a MembershipExpr
+    match (from node select first (RelationOp(f_left: Identifier) |
+                                   MembershipExpr(f_expr: Identifier)))
+    | r@RelationOp     => r.f_left
+    | m@MembershipExpr => m.f_expr
+
 fun check_expr(expr, id, short_circuit) =
     match expr
     # Strip parenthesis
     | ParenExpr                    => check_expr(expr.f_expr, id, short_circuit)
+
     # Check both "or" operands
-    | BinOp(f_op: OpOr | OpOrElse when short_circuit) =>
+    | BinOp(
+        f_op: op@(OpOr | OpOrElse when short_circuit)
+              when stdlib.is_predefined_op(op)
+    ) =>
         check_expr(expr.f_left, id, short_circuit) and
         check_expr(expr.f_right, id, short_circuit)
+
     # Allow X >= E1 and X <= E2
-    | BinOp(f_op: (op@OpAnd when stdlib.is_predefined_op(op)) |
-                    OpAndThen when short_circuit) =>
+    | BinOp(
+        f_op: op@(OpAnd | OpAndThen when short_circuit)
+              when stdlib.is_predefined_op(op)
+    ) =>
         expr.f_left is RelationOp(
             f_op: gte@OpGte,
             f_left: Identifier(p_name_matches(id): true)) when
@@ -19,17 +34,47 @@ fun check_expr(expr, id, short_circuit) =
             f_left: Identifier(p_name_matches(id): true)) when
         stdlib.is_predefined_op(gte) and
         stdlib.is_predefined_op(lte)
+
     # Allow X = C
     | RelationOp(f_op: op@OpEq)  =>
         expr.f_left is Name(p_name_matches(id): true) and
         stdlib.is_predefined_op(op)
+
     # Allow X in ...
     | MembershipExpr(f_op: OpIn) => expr.f_expr.p_name_matches(id)
+
     # Reject anything else
     | *                            => false
 
+fun to_membership_exprs(node) =
+    |" Used by ``membership_equivalent`` to compute the membership format
+    |" of an expression. This function returns a list containing all
+    |" required parts for the created membership expression.
+    match node
+    | ParenExpr => to_membership_exprs(node.f_expr)
+    | BinOp(f_op: (OpOr | OpOrElse)) =>
+        to_membership_exprs(node.f_left) & to_membership_exprs(node.f_right)
+    | BinOp(f_op: (OpAnd | OpAndThen)) =>
+        [new BinOp(
+            f_left=node.f_left.f_right,
+            f_op= new OpDoubleDot(),
+            f_right=node.f_right.f_right
+        )]
+    | RelationOp(f_op: OpEq) => [node.f_right]
+    | MembershipExpr => node.f_membership_exprs.children
+
+fun membership_equivalent(bin_op) =
+    |" Get the membership equivalent of a BinOp ``bin_op`` which has been
+    |" flagged by the "use_memberships" rule.
+    new MembershipExpr(
+        f_expr=get_first_id(bin_op),
+        f_op=new OpIn(),
+        f_membership_exprs=new ExprAlternativesList(to_membership_exprs(bin_op))
+    )
+
 @check(message="expression may be replaced by a membership test",
-       category="Style", subcategory="Programming Practice")
+       category="Style", subcategory="Programming Practice",
+       auto_fix=(n, ctx) => ctx.replace(n, membership_equivalent(n)))
 fun use_memberships(node, short_circuit = false) =
     |" Flag expressions that could be rewritten as membership tests. Only expressions
     |" that are not subexpressions of other expressions are flagged. An expression
@@ -69,14 +114,10 @@ fun use_memberships(node, short_circuit = false) =
     |"                A = B
     |"               or
     |"                A = B + A;
-    node is BinOp(parent: not Expr,
-                  f_op: (op@OpOr when stdlib.is_predefined_op(op)) |
-                          OpOrElse when short_circuit)
-    # Find a first variable used as LHS (id) and pass it to check_expr
-    and match (from node select first
-               (RelationOp(f_left: Identifier)
-               | MembershipExpr(f_expr: Identifier)))
-        | r@RelationOp     => check_expr(node, r.f_left, short_circuit)
-        | m@MembershipExpr => check_expr(node, m.f_expr, short_circuit)
-        | *                => false
+    node is BinOp(
+        parent: not Expr,
+        f_op: op@(OpOr | OpAnd | (OpOrElse | OpAndThen) when short_circuit)
+    ) when stdlib.is_predefined_op(op)
 
+    # Find a first variable used as LHS (id) and pass it to check_expr
+    and check_expr(node, get_first_id(node), short_circuit)
diff --git a/testsuite/tests/checks/use_memberships/member.adb b/testsuite/tests/checks/use_memberships/member.adb
@@ -37,6 +37,7 @@ begin
    end;
 
    Bool := A = B or (A >= 1 and A <= B);  --  FLAG
+   Bool := A >= 1 and A <= B;             --  FLAG
    Bool := A = 100 or A in S;             --  FLAG
    Bool := A = 100 or A in 1 .. B;        --  FLAG
 end Member;
diff --git a/testsuite/tests/checks/use_memberships/test.out b/testsuite/tests/checks/use_memberships/test.out
@@ -18,10 +18,59 @@ member.adb:39:12: rule violation: expression may be replaced by a membership tes
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 member.adb:40:12: rule violation: expression may be replaced by a membership test
-40 |    Bool := A = 100 or A in S;             --  FLAG
+40 |    Bool := A >= 1 and A <= B;             --  FLAG
    |            ^^^^^^^^^^^^^^^^^
 
 member.adb:41:12: rule violation: expression may be replaced by a membership test
-41 |    Bool := A = 100 or A in 1 .. B;        --  FLAG
+41 |    Bool := A = 100 or A in S;             --  FLAG
+   |            ^^^^^^^^^^^^^^^^^
+
+member.adb:42:12: rule violation: expression may be replaced by a membership test
+42 |    Bool := A = 100 or A in 1 .. B;        --  FLAG
    |            ^^^^^^^^^^^^^^^^^^^^^^
 
+Patched "member.adb":
+=====================
+
+procedure Member (A, B : Integer) is
+   Bool : Boolean;
+   subtype S is Integer range 1 .. B;
+begin
+   if A in 0            --  FLAG
+     |Natural
+     |2
+     |3 ..5 then
+      null;
+   end if;
+
+   if A in 0            --  FLAG if short_circuit
+     |3 ..5 then
+      null;
+   end if;
+
+   if A = 0 or (A in Natural and A = 2) then  --  NOFLAG
+      null;
+   end if;
+
+   if A = 0            --  NOFLAG
+     or (A >= 3 and A < 5)
+   then
+      null;
+   end if;
+
+   declare
+      function "or" (A, B : Boolean) return Boolean is (True);
+   begin
+      if A = 0            --  NOFLAG
+        or A = 2
+      then
+         null;
+      end if;
+   end;
+
+   Bool := A in B |1 ..B;  --  FLAG
+   Bool := A in 1 ..B;             --  FLAG
+   Bool := A in 100 |S;             --  FLAG
+   Bool := A in 100 |1 .. B;        --  FLAG
+end Member;
+
diff --git a/testsuite/tests/checks/use_memberships/test.yaml b/testsuite/tests/checks/use_memberships/test.yaml
@@ -3,3 +3,4 @@ rule_name: use_memberships
 project: 'prj.gpr'
 rule_arguments:
    Use_Memberships.short_circuit: 'true'
+auto_fix: True
