From 61270cdaed2267bfffb705f1d16c2855f87f38c5 Mon Sep 17 00:00:00 2001
From: Jamey Sharp <jsharp@fastly.com>
Date: Tue, 31 Jan 2023 09:08:31 -0800
Subject: [PATCH] ISLE: reject multi-term rules with explicit priorities
 (#5663)

In multi-terms, all matching rules fire. We treat the result as an
unordered set of values, so setting rule priorities is meaningless. We
want to prohibit relying on the rule match order in this case.

Also, codegen can produce invalid Rust if rules with different
priorities both match against a multi-term. We first documented this
symptom in #5647. As far as I can figure, prohibiting rule priorities
prevents all possible instances of that bug.

At some point in the future we might decide we want to carefully define
semantics for multi-term result ordering, at which point we can revisit
this.
---
 .../isle/isle/isle_examples/fail/multi_prio.isle   |  4 ++++
 .../isle/isle_examples/link/multi_extractor.isle   |  2 +-
 cranelift/isle/isle/src/sema.rs                    | 14 +++++++++++++-
 cranelift/isle/isle/tests/run_tests.rs             |  8 ++++++--
 4 files changed, 24 insertions(+), 4 deletions(-)
 create mode 100644 cranelift/isle/isle/isle_examples/fail/multi_prio.isle

diff --git a/cranelift/isle/isle/isle_examples/fail/multi_prio.isle b/cranelift/isle/isle/isle_examples/fail/multi_prio.isle
new file mode 100644
index 0000000000..30ce9c7a5f
--- /dev/null
+++ b/cranelift/isle/isle/isle_examples/fail/multi_prio.isle
@@ -0,0 +1,4 @@
+(type u32 (primitive u32))
+
+(decl multi A (u32) u32)
+(rule 0 (A x) x)
diff --git a/cranelift/isle/isle/isle_examples/link/multi_extractor.isle b/cranelift/isle/isle/isle_examples/link/multi_extractor.isle
index 5878dd0652..2630d86826 100644
--- a/cranelift/isle/isle/isle_examples/link/multi_extractor.isle
+++ b/cranelift/isle/isle/isle_examples/link/multi_extractor.isle
@@ -7,7 +7,7 @@
 
 (decl multi Rule (u32) u32)
 
-(rule 1 (Rule (E1 a idx))
+(rule (Rule (E1 a idx))
       (if-let (A.B) a)
       idx)
 (rule (Rule _)
diff --git a/cranelift/isle/isle/src/sema.rs b/cranelift/isle/isle/src/sema.rs
index 95481f1067..ba9906f853 100644
--- a/cranelift/isle/isle/src/sema.rs
+++ b/cranelift/isle/isle/src/sema.rs
@@ -1744,6 +1744,18 @@ impl TermEnv {
 
                     bindings.exit_scope();
 
+                    let prio = if let Some(prio) = rule.prio {
+                        if flags.multi {
+                            tyenv.report_error(
+                                pos,
+                                "Cannot set rule priorities in multi-terms".to_string(),
+                            );
+                        }
+                        prio
+                    } else {
+                        0
+                    };
+
                     let rid = RuleId(self.rules.len());
                     self.rules.push(Rule {
                         id: rid,
@@ -1752,7 +1764,7 @@ impl TermEnv {
                         iflets,
                         rhs,
                         vars: bindings.seen,
-                        prio: rule.prio.unwrap_or(0),
+                        prio,
                         pos,
                     });
                 }
diff --git a/cranelift/isle/isle/tests/run_tests.rs b/cranelift/isle/isle/tests/run_tests.rs
index 9adf51c32d..1b347b9454 100644
--- a/cranelift/isle/isle/tests/run_tests.rs
+++ b/cranelift/isle/isle/tests/run_tests.rs
@@ -15,8 +15,12 @@ pub fn run_pass(filename: &str) {
 }
 
 pub fn run_fail(filename: &str) {
-    if build(filename).is_ok() {
-        panic!("test {} passed unexpectedly", filename);
+    match build(filename) {
+        Ok(_) => panic!("test {} passed unexpectedly", filename),
+        Err(err) => {
+            // Log the actual errors for use with `cargo test -- --nocapture`
+            println!("failed, as expected:\n{:?}", err);
+        }
     }
 }