Make Base Profile result type check specific to entry expressions (#1256

)

Fixes #1246

The current base profile result type check was overly broad, because it
ran the check on all fragments (not just entry expressions).

This was because during incremental compilation, entry expressions were
added to the AST package as a top-level statement. After that point, the
passes could not differentiate between the entry expression and a
top-level statement.

We don't _actually_ have to convert the entry expression to a statement
- we can just keep the entry expression in the `entry` field in the AST
and HIR. So, now, if passes want to apply any special treatment to the
entry expression, they have enough information to do so.

compiler/qsc/src/incremental.rs

@@ -149,12 +149,12 @@ impl Compiler {
     /// get information about the newly added items, or do other modifications.
     /// It is then the caller's responsibility to merge
     /// these packages into the current `CompileUnit` using the `update()` method.
-    pub fn compile_expr(&mut self, expr: &str) -> Result<Increment, Errors> {
+    pub fn compile_entry_expr(&mut self, expr: &str) -> Result<Increment, Errors> {
         let (core, unit) = self.store.get_open_mut();
 
         let mut increment = self
             .frontend
-            .compile_expr(unit, "<entry>", expr)
+            .compile_entry_expr(unit, expr)
             .map_err(into_errors)?;
 
         let pass_errors = self.passes.run_default_passes(
@@ -173,6 +173,7 @@ impl Compiler {
 
     /// Updates the current compilation with the AST and HIR packages,
     /// and any associated context, returned from a previous incremental compilation.
+    /// Entry expressions are ignored.
     pub fn update(&mut self, new: Increment) {
         let (_, unit) = self.store.get_open_mut();
 

compiler/qsc/src/interpret.rs

@@ -300,15 +300,16 @@ impl Interpreter {
         receiver: &mut impl Receiver,
         expr: &str,
     ) -> std::result::Result<InterpretResult, Vec<Error>> {
-        let stmt_id = self.compile_expr_to_stmt(expr)?;
+        let expr_id = self.compile_entry_expr(expr)?;
+
         if self.quantum_seed.is_some() {
             sim.set_seed(self.quantum_seed);
         }
 
         Ok(eval(
             self.package,
             self.classical_seed,
-            stmt_id.into(),
+            expr_id.into(),
             self.compiler.package_store(),
             &self.fir_store,
             &mut Env::default(),
@@ -317,10 +318,21 @@ impl Interpreter {
         ))
     }
 
-    fn compile_expr_to_stmt(&mut self, expr: &str) -> std::result::Result<StmtId, Vec<Error>> {
-        let increment = self.compiler.compile_expr(expr).map_err(into_errors)?;
+    fn compile_entry_expr(&mut self, expr: &str) -> std::result::Result<ExprId, Vec<Error>> {
+        let increment = self
+            .compiler
+            .compile_entry_expr(expr)
+            .map_err(into_errors)?;
+
+        // `lower` will update the entry expression in the FIR store,
+        // and it will always return an empty list of statements.
+        let _ = self.lower(&increment);
 
-        let stmts = self.lower(&increment);
+        // The AST and HIR packages in `increment` only contain an entry
+        // expression and no statements. The HIR *can* contain items if the entry
+        // expression defined any items.
+        assert!(increment.hir.stmts.is_empty());
+        assert!(increment.ast.package.nodes.is_empty());
 
         // Updating the compiler state with the new AST/HIR nodes
         // is not necessary for the interpreter to function, as all
@@ -330,10 +342,10 @@ impl Interpreter {
         // here to keep the package stores consistent.
         self.compiler.update(increment);
 
-        assert!(stmts.len() == 1, "expected exactly one statement");
-        let stmt_id = stmts.first().expect("expected exactly one statement");
+        let unit = self.fir_store.get(self.package);
+        let entry = unit.entry.expect("package should have an entry expression");
 
-        Ok(*stmt_id)
+        Ok(entry)
     }
 
     fn lower(&mut self, unit_addition: &qsc_frontend::incremental::Increment) -> Vec<StmtId> {

compiler/qsc/src/interpret/tests.rs

@@ -1044,6 +1044,20 @@ mod given_interpreter {
                 );
             }
         }
+
+        #[test]
+        fn base_prof_non_result_return() {
+            let mut interpreter = Interpreter::new(
+                true,
+                SourceMap::default(),
+                PackageType::Lib,
+                RuntimeCapabilityFlags::empty(),
+                LanguageFeatures::default(),
+            )
+            .expect("interpreter should be created");
+            let (result, output) = line(&mut interpreter, "123");
+            is_only_value(&result, &output, &Value::Int(123));
+        }
     }
 
     fn get_interpreter() -> Interpreter {

compiler/qsc_eval/src/lower.rs

@@ -94,12 +94,16 @@ impl Lowerer {
             .map(|s| self.lower_stmt(s))
             .collect();
 
+        let entry = hir_package.entry.as_ref().map(|e| self.lower_expr(e));
+
         self.update_package(fir_package);
 
         for (k, v) in items {
             fir_package.items.insert(k, v);
         }
 
+        fir_package.entry = entry;
+
         qsc_fir::validate::validate(fir_package);
 
         new_stmts

compiler/qsc_frontend/src/incremental.rs

@@ -16,7 +16,7 @@ use crate::{
 };
 use qsc_ast::{
     assigner::Assigner as AstAssigner,
-    ast::{self, Stmt, TopLevelNode},
+    ast::{self},
     mut_visit::MutVisitor,
     validate::Validator as AstValidator,
     visit::Visitor as AstVisitor,
@@ -149,18 +149,13 @@ impl Compiler {
     /// get information about the newly added items, or do other modifications.
     /// It is then the caller's responsibility to merge
     /// these packages into the current `CompileUnit`.
-    pub fn compile_expr(
+    pub fn compile_entry_expr(
         &mut self,
         unit: &mut CompileUnit,
-        source_name: &str,
         source_contents: &str,
     ) -> Result<Increment, Vec<Error>> {
-        let (mut ast, parse_errors) = Self::parse_expr(
-            &mut unit.sources,
-            source_name,
-            source_contents,
-            self.language_features,
-        );
+        let (mut ast, parse_errors) =
+            Self::parse_entry_expr(&mut unit.sources, source_contents, self.language_features);
 
         if !parse_errors.is_empty() {
             return Err(parse_errors);
@@ -247,9 +242,6 @@ impl Compiler {
     /// Entry expressions are ignored.
     #[must_use]
     fn concat_ast(&mut self, mut left: ast::Package, right: ast::Package) -> ast::Package {
-        assert!(right.entry.is_none(), "package should not have entry expr");
-        assert!(left.entry.is_none(), "package should not have entry expr");
-
         let mut nodes = Vec::with_capacity(left.nodes.len() + right.nodes.len());
         nodes.extend(left.nodes.into_vec());
         nodes.extend(right.nodes.into_vec());
@@ -260,30 +252,22 @@ impl Compiler {
         left
     }
 
-    fn parse_expr(
+    fn parse_entry_expr(
         sources: &mut SourceMap,
-        source_name: &str,
         source_contents: &str,
         language_features: LanguageFeatures,
     ) -> (ast::Package, Vec<Error>) {
-        let offset = sources.push(source_name.into(), source_contents.into());
+        let offset = sources.push("<entry>".into(), source_contents.into());
 
-        let (expr, errors) = qsc_parse::expr(source_contents, language_features);
-        let mut stmt = Box::new(Stmt {
-            id: ast::NodeId::default(),
-            span: expr.span,
-            kind: Box::new(ast::StmtKind::Expr(expr)),
-        });
+        let (mut expr, errors) = qsc_parse::expr(source_contents, language_features);
 
         let mut offsetter = Offsetter(offset);
-        offsetter.visit_stmt(&mut stmt);
-
-        let top_level_nodes = Box::new([TopLevelNode::Stmt(stmt)]);
+        offsetter.visit_expr(&mut expr);
 
         let package = ast::Package {
             id: ast::NodeId::default(),
-            nodes: top_level_nodes,
-            entry: None,
+            nodes: Box::default(),
+            entry: Some(expr),
         };
 
         (package, with_source(errors, sources, offset))

compiler/qsc_passes/src/baseprofck.rs

@@ -7,12 +7,9 @@ mod tests;
 use miette::Diagnostic;
 use qsc_data_structures::span::Span;
 use qsc_hir::{
-    hir::{
-        BinOp, CallableKind, Expr, ExprKind, Item, ItemKind, Lit, Package, SpecBody, SpecGen,
-        StmtKind,
-    },
+    hir::{BinOp, CallableKind, Expr, ExprKind, Item, ItemKind, Lit, Package, SpecBody, SpecGen},
     ty::{Prim, Ty},
-    visit::{walk_expr, walk_item, walk_package, Visitor},
+    visit::{walk_expr, walk_item, Visitor},
 };
 use thiserror::Error;
 
@@ -65,16 +62,6 @@ struct Checker {
 }
 
 impl<'a> Visitor<'a> for Checker {
-    fn visit_package(&mut self, package: &'a Package) {
-        if let Some(StmtKind::Expr(expr)) = &package.stmts.last().map(|stmt| &stmt.kind) {
-            if any_non_result_ty(&expr.ty) {
-                self.errors.push(Error::ReturnNonResult(expr.span));
-            }
-        }
-
-        walk_package(self, package);
-    }
-
     fn visit_item(&mut self, item: &'a Item) {
         match &item.kind {
             ItemKind::Callable(callable)