Update rules to forbid ambiguous boolean operator precedence

And Or precedence is well defined in general. And is a higher precedence than or and
should be evaluated first, but it causes enough mistakes that I have decided to raise
a parsing error when things like A | B & C happen. Now the user must include some grouping
with [] to specify the precedence
Also with the grammer change, it was easiest to make or and and chain
together, like PipeChain instead of nesting.
This should ideally also be more efficient.

Author: madeline-scyphers

src/rules.lark

@@ -3,22 +3,32 @@
 
 step_expr: expr ("," expr)+               -> step_
 
+# expr can be a pure-or chain, a pure-and chain, or a single pipe_expr
 ?expr: or_expr
+     | and_expr
+     | pipe_expr
 
-?or_expr: and_expr
-        | or_expr "|" and_expr            -> or_
+# OR is a chain of pipe_expr operands (NOT and_expr operands)
+or_expr: pipe_expr ("|" pipe_expr)+       -> or_
 
-?and_expr: pipe_expr
-         | and_expr "&" pipe_expr         -> and_
+# AND is a chain of pipe_expr operands (NOT or_expr operands)
+and_expr: pipe_expr ("&" pipe_expr)+      -> and_
 
 ?pipe_expr: atom
           | pipe_expr "->" call           -> pipe_
 
 ?atom: call
      | literal
      | "[" expr "]"                       -> group
+     | "[" step_expr "]"                  -> group
 
-call: IDENT "(" [expr ("," expr)*] ")"    -> call
+call: IDENT "(" [arg ("," arg)*] ")"    -> call
+
+// list literal as a VALUE (requires at least one comma)
+list_lit: "[" expr ("," expr)+ "]"   -> step_
+
+?arg: list_lit
+    | expr
 
 ?literal: name_ref
         | NUMBER                          -> number

src/rules.py

@@ -9,7 +9,7 @@
 
 import numpy as np
 import polars as pl
-from lark import Lark, Transformer
+from lark import Lark, Transformer, LarkError
 
 OP_TO_EXPR = {
     "gt": operator.gt,
@@ -89,6 +89,12 @@ class Expr:
     def validate(self):
         pass
 
+    def pprint(self):
+        """Pretty print the expression for debugging."""
+        from pprint import pprint
+
+        pprint(self)
+
 
 @dataclass(frozen=True)
 class Name(Expr):
@@ -108,14 +114,12 @@ class String(Expr):
 
 @dataclass(frozen=True)
 class And(Expr):
-    a: Expr
-    b: Expr
+    parts: Tuple[Expr, ...]
 
 
 @dataclass(frozen=True)
 class Or(Expr):
-    a: Expr
-    b: Expr
+    parts: Tuple[Expr, ...]
 
 
 @dataclass(frozen=True)
@@ -247,13 +251,14 @@ def string(self, items):
         return String(s)
 
     def and_(self, items):
-        return And(items[0], items[1])
+        return And(parts=tuple(items))
 
     def or_(self, items):
-        return Or(items[0], items[1])
+        return Or(parts=tuple(items))
 
     def group(self, items):
         # square brackets are just grouping, not a node
+        assert len(items) == 1, "Grouping isn't supposed to change number of items"
         return items[0]
 
     def step_(self, items):
@@ -297,7 +302,6 @@ def from_rules(cls, *args, **kwargs) -> CompiledRules:
             k: expand_macros(v, defs_expanded, needed_features=needed_features)
             for k, v in rules.items()
         }
-
         return cls(rules=rules_expanded, needed_features=needed_features)
 
 
@@ -338,11 +342,32 @@ def load_rules(
     with open(Path(__file__).parent.absolute() / "rules.lark") as f:
         parser = Lark(f, parser="lalr", transformer=ASTTransformer())
 
+    def parse_rule_expr(expr_str: str) -> Expr:
+        """We use a closure to capture the parser instance since
+        polars map_elements doesn't let us pass extra args."""
+        try:
+            return parser.parse(expr_str)
+        except LarkError as e:
+            n_left_b = expr_str.count("[")
+            n_right_b = expr_str.count("]")
+            if n_left_b != n_right_b:
+                raise RuleError(
+                    f"Possible mismatched brackets in rule expression: {expr_str}"
+                ) from e
+            if expr_str.count("&") > 0 and expr_str.count("|") > 0:
+                # both used; likely missing brackets
+                raise RuleError(
+                    f"Possible ambiguous use of '&' or '|' without surrounding brackets `[ ]`. Check that you don't have a situation like `A | B & C` or `A & B | C`. These are generally not allowed because they can be ambiguous. These should be written as `[A | B] & C` or `[A & B] | C`: {expr_str}."
+                ) from e
+            raise RuleError(
+                f"Error parsing rule expression for unknown reason. Possible hints could be found in the above exceptions from the parsing library Lark: {expr_str}"
+            ) from e
+
     lf = lf.with_columns(
         [
             pl.col(rules_col)
             .str.strip_chars()
-            .map_elements(parser.parse, return_dtype=pl.Object)
+            .map_elements(parse_rule_expr, return_dtype=pl.Object)
         ]
     )
 
@@ -400,9 +425,9 @@ def recurse(e: Expr, add_name_to_needed: bool = True) -> Expr:
         elif isinstance(e, (Number, String)):
             out = e
         elif isinstance(e, And):
-            out = And(recurse(e.a), recurse(e.b))
+            out = And(parts=tuple(recurse(part) for part in e.parts))
         elif isinstance(e, Or):
-            out = Or(recurse(e.a), recurse(e.b))
+            out = Or(parts=tuple(recurse(part) for part in e.parts))
         elif isinstance(e, Steps):
             out = Steps(tuple(recurse(p) for p in e.parts))
         elif isinstance(e, Call):
@@ -529,13 +554,18 @@ def eval_bool(self, expr: Expr) -> np.ndarray:
             return out
 
         if isinstance(expr, And):
-            out = np.logical_and(self.eval_bool(expr.a), self.eval_bool(expr.b))
+            out = np.all(
+                np.stack([self.eval_bool(part) for part in expr.parts], axis=0), axis=0
+            )
             self._memo[expr] = out
             return out
 
         if isinstance(expr, Or):
             try:
-                out = np.logical_or(self.eval_bool(expr.a), self.eval_bool(expr.b))
+                out = np.any(
+                    np.stack([self.eval_bool(part) for part in expr.parts], axis=0),
+                    axis=0,
+                )
             except ValueError as e:
                 print(f"Error evaluating OR expression: {expr}")
                 raise
@@ -565,10 +595,10 @@ def eval_bool(self, expr: Expr) -> np.ndarray:
 
         if isinstance(expr, Steps):
             raise RuleError(
-                f"Step expressions (expression seperated by commas)"
+                "Step expressions (expression seperated by commas)"
                 " logic cannot be evaluted on their own. They must be used"
                 " in a supporting function such as `percent`. Offending"
-                " rule: {expr}"
+                f" rule: {expr}"
             )
 
         raise RuleError(

tests/data/basic_anno.tsv

@@ -1,7 +1,7 @@
 query_id	input_fasta	kegg_id	kegg_description	sulfur_id	fegenie_id	kofam_id	merops_id	methyl_id	dbcan_id	camper_id	heme_regulatory_motif_count
-bin1_1	bin1	K00399	mez:Mtc_0908 (K00399) mcrA; methyl-coenzyme M reductase alpha subunit [EC:2.8.4.1]			K00399					0
+bin1_1	bin1	K00399	mez:Mtc_0908 (K00399) mcrA; methyl-coenzyme M reductase alpha subunit [EC:2.8.4.1]			K00399					5
 bin1_2	bin1	K14126	"mez:Mtc_2479 (K14126) mvhA, vhuA, vhcA; F420-non-reducing hydrogenase large subunit [EC:1.12.99.- 1.8.98.5]"			K14126					4
-bin1_3	bin1	K14128	"mez:Mtc_2478 (K14128) mvhG, vhuG, vhcG; F420-non-reducing hydrogenase small subunit [EC:1.12.99.- 1.8.98.5]"			K14128					2
+bin1_3	bin1	K14128	"mez:Mtc_2478 (K14128) mvhG, vhuG, vhcG; F420-non-reducing hydrogenase small subunit [EC:1.12.99.- 1.8.98.5]"			K14128					4
 bin1_4	bin1	K00205	"rci:RCIX592 (K00205) fwdF, fmdF; 4Fe-4S ferredoxin"			K00205					0
 bin2_1	bin2	K03390	mla:Mlab_0239 (K03390) hdrC2; heterodisulfide reductase subunit C2 [EC:1.8.7.3 1.8.98.4 1.8.98.5 1.8.98.6]			K03390	MER0283993				0
 bin2_2	bin2	K03389	mez:Mtc_2474 (K03389) hdrB2; heterodisulfide reductase subunit B2 [EC:1.8.7.3 1.8.98.4 1.8.98.5 1.8.98.6]			K03389					0

tests/data/compound_rules.tsv

@@ -6,9 +6,9 @@ B	b1	b2 | b4
 	b2	percent(50,b3)
 	b3	K00399,[K00401 & K00402 & [K00333 | K03421] & [K03422 | K13380]],K00400,K00401,K00402,K03421,K03422
 	b4	K02591 | K02586 | K02588
-C	c1	c1 & c2
-	c1	not(c3)
-	c2	column_count_values(heme_regulatory_motif_count,ge,4,ge,3)
-	c3	column_contains(kegg_description,NitrateReducer)
+C	c1	c2
+	c2	c3 -> column_count_values(heme_regulatory_motif_count,ge,2,ge,2)
+	c3	not(c4)
+	c4	filter_contains(kegg_description,"nitrate reductase")
 D	d1	percent(50,d2)
 	d2	K00399,K14126,K02588