Drop FIR increment if FIR passes fail (#1559)

This fixes an issue with incremental compilation where an increment with
a failure could still have that FIR present in the package during later
compilation, making errors "sticky" instead of associated with just the
increment. This achieves it by tracking what the most recently lowered
additions to the package were and removing them if there were pass
failures like those from RCA check.

compiler/qsc/src/interpret.rs

@@ -618,7 +618,10 @@ impl Interpreter {
 
         let compute_properties = cap_results.map_err(|caps_errors| {
             // if there are errors, convert them to interpreter errors
-            // and don't update the lowerer or FIR store.
+            // and revert the update to the lowerer/FIR store.
+            let fir_package = self.fir_store.get_mut(self.package);
+            self.lowerer.revert_last_increment(fir_package);
+
             let source_package = self
                 .compiler
                 .package_store()

compiler/qsc/src/interpret/tests.rs

@@ -517,69 +517,116 @@ mod given_interpreter {
         }
 
         #[test]
-        fn callables_failing_profile_validation_are_still_registered() {
-            fn verify_same_error<E>(result: &Result<Value, Vec<E>>, output: &str)
-            where
-                E: Diagnostic,
-            {
-                is_only_error(
-                    result,
-                    output,
-                    &expect![[r#"
-                    cannot use a dynamic integer value
-                       [line_0] [set x = 2]
-                    cannot use a dynamic integer value
-                       [line_0] [x]
-                "#]],
-                );
-            }
+        fn callables_failing_profile_validation_are_not_registered() {
             let mut interpreter = get_interpreter_with_capbilities(TargetCapabilityFlags::Adaptive);
             let (result, output) = line(
                 &mut interpreter,
                 indoc! {r#"
                     operation Foo() : Int { use q = Qubit(); mutable x = 1; if MResetZ(q) == One { set x = 2; } x }
                 "#},
             );
-            verify_same_error(&result, &output);
+            is_only_error(
+                &result,
+                &output,
+                &expect![[r#"
+                cannot use a dynamic integer value
+                   [line_0] [set x = 2]
+                cannot use a dynamic integer value
+                   [line_0] [x]
+            "#]],
+            );
             // do something innocuous
             let (result, output) = line(&mut interpreter, indoc! {r#"Foo()"#});
-            // if the callable wasn't registered, this would panic instead of returning an error.
-            verify_same_error(&result, &output);
+            // since the callable wasn't registered, this will return an unbound name error.
+            is_only_error(
+                &result,
+                &output,
+                &expect![[r#"
+                runtime error: name is not bound
+                   [line_1] [Foo]
+            "#]],
+            );
         }
 
         #[test]
-        fn once_rca_validation_fails_following_calls_also_fail_by_design() {
-            fn verify_same_error<E>(result: &Result<Value, Vec<E>>, output: &str)
-            where
-                E: Diagnostic,
-            {
-                is_only_error(
-                    result,
-                    output,
-                    &expect![[r#"
-                    cannot use a dynamic integer value
-                       [line_0] [set x = 2]
-                    cannot use a dynamic integer value
-                       [line_0] [x]
-                "#]],
-                );
-            }
+        fn callables_failing_profile_validation_also_fail_qir_generation() {
             let mut interpreter = get_interpreter_with_capbilities(TargetCapabilityFlags::Adaptive);
             let (result, output) = line(
                 &mut interpreter,
                 indoc! {r#"
                     operation Foo() : Int { use q = Qubit(); mutable x = 1; if MResetZ(q) == One { set x = 2; } x }
                 "#},
             );
-            verify_same_error(&result, &output);
+            is_only_error(
+                &result,
+                &output,
+                &expect![[r#"
+                cannot use a dynamic integer value
+                   [line_0] [set x = 2]
+                cannot use a dynamic integer value
+                   [line_0] [x]
+            "#]],
+            );
+            let res = interpreter.qirgen("{Foo();}");
+            expect![[r#"
+                Err(
+                    [
+                        PartialEvaluation(
+                            WithSource {
+                                sources: [
+                                    Source {
+                                        name: "<entry>",
+                                        contents: "{Foo();}",
+                                        offset: 97,
+                                    },
+                                ],
+                                error: EvaluationFailed(
+                                    "name is not bound",
+                                    PackageSpan {
+                                        package: PackageId(
+                                            3,
+                                        ),
+                                        span: Span {
+                                            lo: 98,
+                                            hi: 101,
+                                        },
+                                    },
+                                ),
+                            },
+                        ),
+                    ],
+                )
+            "#]]
+            .assert_debug_eq(&res);
+        }
+
+        #[test]
+        fn once_rca_validation_fails_following_calls_do_not_fail() {
+            let mut interpreter = get_interpreter_with_capbilities(TargetCapabilityFlags::Adaptive);
+            let (result, output) = line(
+                &mut interpreter,
+                indoc! {r#"
+                    operation Foo() : Int { use q = Qubit(); mutable x = 1; if MResetZ(q) == One { set x = 2; } x }
+                "#},
+            );
+            is_only_error(
+                &result,
+                &output,
+                &expect![[r#"
+                cannot use a dynamic integer value
+                   [line_0] [set x = 2]
+                cannot use a dynamic integer value
+                   [line_0] [x]
+            "#]],
+            );
             // do something innocuous
             let (result, output) = line(
                 &mut interpreter,
                 indoc! {r#"
                     let y = 7;
                 "#},
             );
-            verify_same_error(&result, &output);
+            is_only_value(&result, &output, &Value::unit());
         }
 
         #[test]

compiler/qsc_lowerer/src/lib.rs

@@ -22,6 +22,15 @@ pub fn map_fir_package_to_hir(package: fir::PackageId) -> hir::PackageId {
     hir::PackageId::from(Into::<usize>::into(package))
 }
 
+#[derive(Debug, Default)]
+struct FirIncrement {
+    blocks: Vec<BlockId>,
+    exprs: Vec<ExprId>,
+    pats: Vec<PatId>,
+    stmts: Vec<StmtId>,
+    items: Vec<LocalItemId>,
+}
+
 pub struct Lowerer {
     nodes: IndexMap<hir::NodeId, fir::NodeId>,
     locals: IndexMap<hir::NodeId, fir::LocalVarId>,
@@ -33,6 +42,7 @@ pub struct Lowerer {
     exec_graph: Vec<ExecGraphNode>,
     enable_debug: bool,
     ret_node: ExecGraphNode,
+    fir_increment: FirIncrement,
 }
 
 impl Default for Lowerer {
@@ -55,6 +65,7 @@ impl Lowerer {
             exec_graph: Vec::new(),
             enable_debug: false,
             ret_node: ExecGraphNode::Ret,
+            fir_increment: FirIncrement::default(),
         }
     }
 
@@ -118,6 +129,9 @@ impl Lowerer {
         fir_package: &mut fir::Package,
         hir_package: &hir::Package,
     ) {
+        // Clear the previous increment since we are about to take a new one.
+        self.fir_increment = FirIncrement::default();
+
         let items: IndexMap<LocalItemId, fir::Item> = hir_package
             .items
             .values()
@@ -135,28 +149,51 @@ impl Lowerer {
 
         for (k, v) in items {
             fir_package.items.insert(k, v);
+            self.fir_increment.items.push(k);
         }
 
         fir_package.entry = entry;
 
         qsc_fir::validate::validate(fir_package);
     }
 
+    pub fn revert_last_increment(&mut self, package: &mut fir::Package) {
+        for id in self.fir_increment.blocks.drain(..) {
+            package.blocks.remove(id);
+        }
+        for id in self.fir_increment.exprs.drain(..) {
+            package.exprs.remove(id);
+        }
+        for id in self.fir_increment.pats.drain(..) {
+            package.pats.remove(id);
+        }
+        for id in self.fir_increment.stmts.drain(..) {
+            package.stmts.remove(id);
+        }
+        for id in self.fir_increment.items.drain(..) {
+            package.items.remove(id);
+        }
+    }
+
     fn update_package(&mut self, package: &mut fir::Package) {
         for (id, value) in self.blocks.drain() {
             package.blocks.insert(id, value);
+            self.fir_increment.blocks.push(id);
         }
 
         for (id, value) in self.exprs.drain() {
             package.exprs.insert(id, value);
+            self.fir_increment.exprs.push(id);
         }
 
         for (id, value) in self.pats.drain() {
             package.pats.insert(id, value);
+            self.fir_increment.pats.push(id);
         }
 
         for (id, value) in self.stmts.drain() {
             package.stmts.insert(id, value);
+            self.fir_increment.stmts.push(id);
         }
     }
 

compiler/qsc_rca/src/core.rs

@@ -547,10 +547,12 @@ impl<'a> Analyzer<'a> {
         };
 
         // We could resolve the callee. Determine the compute kind of the call depending on the callee kind.
-        let global_callee = self
-            .package_store
-            .get_global(callee.item)
-            .expect("global should exist");
+        let Some(global_callee) = self.package_store.get_global(callee.item) else {
+            // If the callee is not found, that is an indication that it is an item that was removed during
+            // incremental compilation but remains in the name resolution data structures. Assume it is classical
+            // so that it generates an "unbound name" error at runtime.
+            return CallComputeKind::Regular(ComputeKind::Classical);
+        };
         match global_callee {
             Global::Callable(callable_decl) => self.analyze_expr_call_with_spec_callee(
                 &callee,

pip/tests/test_interpreter.py

@@ -274,30 +274,44 @@ def test_entry_expr_circuit() -> None:
     )
 
 
-# this is by design
-def test_callables_failing_profile_validation_are_still_registered() -> None:
+def test_callables_failing_profile_validation_are_not_registered() -> None:
     e = Interpreter(TargetProfile.Adaptive_RI)
     with pytest.raises(Exception) as excinfo:
         e.interpret(
             "operation Foo() : Double { use q = Qubit(); mutable x = 1.0; if MResetZ(q) == One { set x = 2.0; } x }"
         )
     assert "Qsc.CapabilitiesCk.UseOfDynamicDouble" in str(excinfo)
+    # In this case, the callable Foo failed compilation late enough that the symbol is bound. This makes later
+    # use of `Foo` valid from a name resolution standpoint, but the callable cannot be invoked because it was found
+    # to be invalid for the current profile. To stay consistent with the behavior of other compilations that
+    # leave unbound symbols, the call will compile but fail to run.
     with pytest.raises(Exception) as excinfo:
         e.interpret("Foo()")
-    assert "Qsc.CapabilitiesCk.UseOfDynamicDouble" in str(excinfo)
+    assert "Qsc.Eval.UnboundName" in str(excinfo)
 
 
-# this is by design
-def test_once_rca_validation_fails_following_calls_also_fail() -> None:
+def test_once_callable_fails_profile_validation_it_fails_compile_to_QIR() -> None:
     e = Interpreter(TargetProfile.Adaptive_RI)
     with pytest.raises(Exception) as excinfo:
         e.interpret(
             "operation Foo() : Double { use q = Qubit(); mutable x = 1.0; if MResetZ(q) == One { set x = 2.0; } x }"
         )
     assert "Qsc.CapabilitiesCk.UseOfDynamicDouble" in str(excinfo)
     with pytest.raises(Exception) as excinfo:
-        e.interpret("let x = 5;")
+        e.qir("{Foo();}")
+    assert "Qsc.PartialEval.EvaluationFailed" in str(excinfo)
+    assert "name is not bound" in str(excinfo)
+
+
+def test_once_rca_validation_fails_following_calls_do_not_fail() -> None:
+    e = Interpreter(TargetProfile.Adaptive_RI)
+    with pytest.raises(Exception) as excinfo:
+        e.interpret(
+            "operation Foo() : Double { use q = Qubit(); mutable x = 1.0; if MResetZ(q) == One { set x = 2.0; } x }"
+        )
     assert "Qsc.CapabilitiesCk.UseOfDynamicDouble" in str(excinfo)
+    value = e.interpret("let x = 5; x")
+    assert value == 5
 
 
 def test_adaptive_errors_are_raised_when_interpreting() -> None: