Repository: hylang/hy

Issue:
Optimize and/or
Depends on #824.
Related #842.

Even in the case that `and`/`or` contains a statement, it may have several expressions in a row that don't need to be converted to `if` branches. Hy could avoid redundant assignments by keeping those runs as expressions and only assigning the result to an anonymous variable.

For example:

``` Python
=> (print (and 1 2 3 (setv x 1) 4 5 6))
_hy_anon_var_1 = 1 and 2 and 3
if _hy_anon_var_1:
    x = 1
    _hy_anon_var_1 = x and 4 and 5 and 6
print(_hy_anon_var_1)
6
```


Discussion:

Patch:
diff --git a/NEWS.rst b/NEWS.rst
index 4b3e7082..969f8aad 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -34,6 +34,7 @@ Misc. Improvements
 ------------------------------
 * Some syntax errors raised by core macros now have more informative
   messages.
+* Logical operators now compile to simpler Python code in some cases.
 
 Bug Fixes
 ------------------------------
diff --git a/hy/core/result_macros.py b/hy/core/result_macros.py
index 81c24ffe..9ed6c65e 100644
--- a/hy/core/result_macros.py
+++ b/hy/core/result_macros.py
@@ -256,48 +256,66 @@ def compile_unary_operator(compiler, expr, root, arg):
 def compile_logical_or_and_and_operator(compiler, expr, operator, args):
     ops = {"and": (ast.And, True), "or": (ast.Or, None)}
     opnode, default = ops[operator]
-    osym = expr[0]
     if len(args) == 0:
-        return asty.Constant(osym, value=default)
-    elif len(args) == 1:
-        return compiler.compile(args[0])
-    ret = Result()
-    values = list(map(compiler.compile, args))
-    if any(value.stmts for value in values):
-        # Compile it to an if...else sequence
-        var = compiler.get_anon_var()
-        name = asty.Name(osym, id=var, ctx=ast.Store())
-        expr_name = asty.Name(osym, id=var, ctx=ast.Load())
-        temp_variables = [name, expr_name]
-
-        def make_assign(value, node=None):
-            positioned_name = asty.Name(node or osym, id=var, ctx=ast.Store())
-            temp_variables.append(positioned_name)
-            return asty.Assign(node or osym, targets=[positioned_name], value=value)
-
-        current = root = []
-        for i, value in enumerate(values):
-            if value.stmts:
-                node = value.stmts[0]
-                current.extend(value.stmts)
+        return asty.Constant(expr[0], value=default)
+
+    ret = None
+    var = None         # A temporary variable for assigning results to
+    assignment = None  # The current assignment to `var`
+    stmts = None       # The current statement list
+
+    def put(node, value):
+        # Save the result of the operation so far to `var`.
+        nonlocal var, assignment
+        if var is None:
+            var = compiler.get_anon_var()
+        name = asty.Name(node, id=var, ctx=ast.Store())
+        ret.temp_variables.append(name)
+        return (assignment := asty.Assign(node, targets=[name], value=value))
+
+    def get(node):
+        # Get the value of `var`, creating it if necessary.
+        if var is None:
+            stmts.append(put(node, ret.force_expr))
+        name = asty.Name(node, id=var, ctx=ast.Load())
+        ret.temp_variables.append(name)
+        return name
+
+    for value in map(compiler.compile, args):
+        if ret is None:
+            # This is the first iteration. Don't actually introduce a
+            # `BoolOp` yet; the unary case doesn't need it.
+            ret = value
+            stmts = ret.stmts
+        elif value.stmts:
+            # Save the result of the statements to the temporary
+            # variable. Use an `if` statement to implement
+            # short-circuiting from this point.
+            node = value.stmts[0]
+            cond = get(node)
+            if operator == "or":
+                # Negate the conditional.
+                cond = asty.UnaryOp(node, op=ast.Not(), operand=cond)
+            branch = asty.If(node, test=cond, body=value.stmts, orelse=[])
+            stmts.append(branch)
+            stmts = branch.body
+            stmts.append(put(node, value.force_expr))
+        else:
+            # Add this value to the current `BoolOp`, or create a new
+            # one if we don't have one.
+            value = value.force_expr
+            def enbool(expr):
+                if isinstance(expr, ast.BoolOp):
+                    expr.values.append(value)
+                    return expr
+                return asty.BoolOp(expr, op=opnode(), values=[expr, value])
+            if assignment:
+                assignment.value = enbool(assignment.value)
             else:
-                node = value.expr
-            current.append(make_assign(value.force_expr, value.force_expr))
-            if i == len(values) - 1:
-                # Skip a redundant 'if'.
-                break
-            if operator == "and":
-                cond = expr_name
-            elif operator == "or":
-                cond = asty.UnaryOp(node, op=ast.Not(), operand=expr_name)
-            current.append(asty.If(node, test=cond, body=[], orelse=[]))
-            current = current[-1].body
-        ret = sum(root, ret)
-        ret += Result(expr=expr_name, temp_variables=temp_variables)
-    else:
-        ret += asty.BoolOp(
-            osym, op=opnode(), values=[value.force_expr for value in values]
-        )
+                ret.expr = enbool(ret.expr)
+
+    if var:
+        ret.expr = get(expr)
     return ret
 
 


Test patch:
diff --git a/tests/compilers/test_ast.py b/tests/compilers/test_ast.py
index 27cf3505..001c2e55 100644
--- a/tests/compilers/test_ast.py
+++ b/tests/compilers/test_ast.py
@@ -449,6 +449,15 @@ def test_literal_newlines():
     assert s("#[[\rhello\rworld]]") == "hello\nworld"
 
 
+def test_linear_boolop():
+   """Operations like `(and 1 2 3)` should use only one `BoolOp`,
+   instead of an equivalent nested sequence of `BoolOp`s."""
+   for op in ("and", "or"):
+       node = can_compile(f'({op} 1 2.0 True "hi" 5)').body[0].value
+       assert len(node.values) == 5
+       assert all(isinstance(v, ast.Constant) for v in node.values)
+
+
 def test_compile_error():
     """Ensure we get compile error in tricky cases"""
     with pytest.raises(HyLanguageError) as excinfo:
diff --git a/tests/compilers/test_compiler.py b/tests/compilers/test_compiler.py
index 64fd2a01..4269fe0d 100644
--- a/tests/compilers/test_compiler.py
+++ b/tests/compilers/test_compiler.py
@@ -3,6 +3,8 @@ import types
 
 from hy import compiler
 from hy.models import Expression, Integer, List, Symbol
+from hy.reader import read_many
+from hy.compiler import hy_compile
 
 
 def make_expression(*args):
@@ -14,6 +16,10 @@ def make_expression(*args):
     return h.replace(h)
 
 
+def hy2py(s):
+    return ast.unparse(hy_compile(read_many(s), "test", import_stdlib=False))
+
+
 def test_compiler_bare_names():
     """
     Check that the compiler doesn't drop bare names from code branches
@@ -58,3 +64,15 @@ def test_compiler_yield_return():
     assert isinstance(body[0].value, ast.Yield)
     assert isinstance(body[1], ast.Return)
     assert isinstance(body[1].value, ast.BinOp)
+
+
+# https://github.com/hylang/hy/issues/854
+def test_compact_logic():
+    """
+    Check that we don't generate multiple unnecessary if-statements
+    when compiling the short-circuiting operators.
+    """
+    py = hy2py("(and 1 2 3 (do (setv x 4) x) 5 6)")
+    assert py.count("if") == 1
+    py = hy2py("(or 1 2 3 (do (setv x 4) x) 5 6 (do (setv y 7)))")
+    assert py.count("if") == 2