Add lint warning for ambiguous if-statement followed by unary operator (#2759)

Because we support unary `+` and `-` a user can mistakenly write an
if-statement followed by something like ` - x` and expect that it
subtracts that from the result of the if-statement, when in fact parser
treats this as two separate statements. This link helps call attention
to the ambiguity and encourage the user to use clearer syntax to
disambiguate.

Author: swernli

source/compiler/qsc_linter/src/lints/hir.rs

@@ -4,8 +4,8 @@
 use qsc_data_structures::span::Span;
 use qsc_hir::{
     hir::{
-        BinOp, CallableDecl, CallableKind, Expr, ExprKind, Field, ItemKind, Res, SpecBody,
-        SpecDecl, Stmt, StmtKind,
+        BinOp, Block, CallableDecl, CallableKind, Expr, ExprKind, Field, ItemKind, Res, SpecBody,
+        SpecDecl, Stmt, StmtKind, UnOp,
     },
     ty::{Prim, Ty},
     visit::{self, Visitor},
@@ -40,6 +40,7 @@ declare_hir_lints! {
     (DeprecatedFunctionConstructor, LintLevel::Allow, "deprecated function constructors", "function constructors for struct types are deprecated, use `new` instead"),
     (DeprecatedWithOperator, LintLevel::Allow, "deprecated `w/` and `w/=` operators for structs", "`w/` and `w/=` operators for structs are deprecated, use `new` instead"),
     (DeprecatedDoubleColonOperator, LintLevel::Allow, "deprecated `::` for field access", "`::` operator is deprecated, use `.` instead"),
+    (AmbiguousUnaryOperatorAfterIf, LintLevel::Warn, "ambiguous unary operator after if-expression", "consider wrapping the if-expression in parentheses or using a semicolon to clarify the intended use of the operator"),
 }
 
 #[derive(Default)]
@@ -342,3 +343,37 @@ impl HirLintPass for DeprecatedDoubleColonOperator {
         }
     }
 }
+
+#[derive(Default)]
+struct AmbiguousUnaryOperatorAfterIf {
+    level: LintLevel,
+}
+
+/// Creates a lint for ambiguous unary operators after if-statements.
+/// For example:
+/// ```qsharp
+/// if condition { a } else { b } - c
+/// ```
+/// The user likely intended this to subtract `c` from the result of the if-expression,
+/// but to achieve this they need to wrap the if-expression in parentheses, like so:
+/// ```qsharp
+/// (if condition { a } else { b }) - c
+/// ```
+impl HirLintPass for AmbiguousUnaryOperatorAfterIf {
+    fn check_block(&mut self, block: &Block, buffer: &mut Vec<Lint>, _compilation: Compilation) {
+        if block.stmts.len() < 2 {
+            // Not enough statements to have an if-expression followed by a unary operator.
+            return;
+        }
+        for i in 0..(block.stmts.len() - 1) {
+            if let StmtKind::Expr(expr) = &block.stmts[i].kind
+                && matches!(&expr.kind, ExprKind::If(..))
+                && let StmtKind::Expr(next_expr) = &block.stmts[i + 1].kind
+                && matches!(next_expr.kind, ExprKind::UnOp(UnOp::Pos | UnOp::Neg, _))
+                && expr.ty != Ty::UNIT
+            {
+                buffer.push(lint!(self, next_expr.span));
+            }
+        }
+    }
+}

source/compiler/qsc_linter/src/tests.rs

@@ -809,6 +809,92 @@ fn deprecated_assign_update_expr_code_action() {
     );
 }
 
+#[test]
+fn ambiguous_unary_operator_after_if() {
+    check(
+        &wrap_in_callable("if true { 42 } else { 0 } - 1", CallableKind::Function),
+        &expect![[r#"
+            [
+                SrcLint {
+                    source: "- 1",
+                    level: Warn,
+                    message: "ambiguous unary operator after if-expression",
+                    help: "consider wrapping the if-expression in parentheses or using a semicolon to clarify the intended use of the operator",
+                    code_action_edits: [],
+                },
+            ]
+        "#]],
+    );
+}
+
+#[test]
+fn ambiguous_unary_operator_after_if_fixed_by_parens_around_if() {
+    check(
+        &wrap_in_callable("(if true { 42 } else { 0 }) - 1", CallableKind::Function),
+        &expect![[r#"
+            []
+        "#]],
+    );
+}
+
+#[test]
+fn ambiguous_unary_operator_after_if_fixed_by_parens_around_all() {
+    check(
+        &wrap_in_callable("(if true { 42 } else { 0 } - 1)", CallableKind::Function),
+        &expect![[r#"
+            []
+        "#]],
+    );
+}
+
+#[test]
+fn ambiguous_unary_operator_after_if_fixed_by_semicolon() {
+    check(
+        &wrap_in_callable("if true { 42 } else { 0 }; - 1", CallableKind::Function),
+        &expect![[r#"
+            []
+        "#]],
+    );
+}
+
+#[test]
+fn ambiguous_unary_operator_after_if_does_trigger_warning_on_different_non_unit_types() {
+    check(
+        &wrap_in_callable("if true { 42.0 } else { 0.0 } - 1", CallableKind::Function),
+        &expect![[r#"
+            [
+                SrcLint {
+                    source: "- 1",
+                    level: Warn,
+                    message: "ambiguous unary operator after if-expression",
+                    help: "consider wrapping the if-expression in parentheses or using a semicolon to clarify the intended use of the operator",
+                    code_action_edits: [],
+                },
+            ]
+        "#]],
+    );
+}
+
+#[test]
+fn ambiguous_unary_operator_after_if_does_not_trigger_on_unit_type() {
+    check(
+        &wrap_in_callable("if true { 42; } else { 0; } - 1", CallableKind::Function),
+        &expect![[r#"
+            []
+        "#]],
+    );
+}
+
+#[test]
+fn ambiguous_unary_operator_after_if_does_not_trigger_for_unary_ops_besides_minus_plus() {
+    check(
+        &wrap_in_callable("if true { 42 } else { 0 } !1", CallableKind::Function),
+        &expect![[r#"
+            []
+        "#]],
+    );
+}
+
 #[test]
 fn check_that_hir_lints_are_deduplicated_in_operations_with_multiple_specializations() {
     check_with_deduplication(

source/vscode/qsharp.schema.json

@@ -65,7 +65,8 @@
                   "needlessOperation",
                   "deprecatedFunctionConstructor",
                   "deprecatedWithOperator",
-                  "deprecatedDoubleColonOperator"
+                  "deprecatedDoubleColonOperator",
+                  "ambiguousUnaryOperatorAfterIf"
                 ]
               },
               "level": {