Skip to content

Commit

Permalink
[Unitary-Hack] Lint rule: Use functions (#1579)
Browse files Browse the repository at this point in the history
Fixes #1471.

### Future TODO
- #1585
- #1593


![image](https://github.com/microsoft/qsharp/assets/40084144/3c512530-ac54-4f9b-85f5-b6327917b7b2)

![image](https://github.com/microsoft/qsharp/assets/40084144/9eeec703-c92e-4906-aecf-91ace0b594d2)


Based on
https://github.com/microsoft/qsharp/blob/main/compiler/qsc_passes/src/callable_limits.rs

### Note to reviewers
Main changes are in `qsc_linter` folder. Changes in other tests are due
to this lint rule complaining about NeedlessOperation, so I converted to
function and it changes span accordingly.

---------

Co-authored-by: orpuente-MS <[email protected]>
Co-authored-by: Mine Starks <[email protected]>
  • Loading branch information
3 people authored Jun 10, 2024
1 parent eaa558a commit 2454f09
Show file tree
Hide file tree
Showing 7 changed files with 250 additions and 46 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions compiler/qsc_linter/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ thiserror = { workspace = true }

[dev-dependencies]
expect-test = { workspace = true }
indoc = { workspace = true }
qsc_parse = { path = "../qsc_parse" }
serde_json = { workspace = true }
qsc = { path = "../qsc" }
Expand Down
101 changes: 95 additions & 6 deletions compiler/qsc_linter/src/lints/hir.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,106 @@
use qsc_hir::hir::Lit;
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

use qsc_hir::{
hir::{CallableDecl, CallableKind, Expr, ExprKind, SpecBody, SpecDecl, Stmt, StmtKind},
ty::Ty,
visit::{self, Visitor},
};

use crate::linter::hir::declare_hir_lints;

use super::lint;

declare_hir_lints! {
(Placeholder, LintLevel::Allow, "this a placeholder", "remove after addding the first HIR lint"),
(NeedlessOperation, LintLevel::Warn, "operation does not contain any quantum operations", "this callable can be declared as a function instead")
}

/// Helper to check if an operation has desired operation characteristics
///
/// empty operations: no lint, special case of `I` operation used for Delay
/// operations with errors (e.g. partially typed code): no lint because linter does not run
/// non-empty operations, with specializations, and no quantum operations: show lint, but don't offer quickfix (to avoid deleting user code in any explicit specializations)
/// non-empty operations with no specializations, and no quantum operations: show lint, offer quickfix to convert to function
#[derive(Default)]
struct IsQuantumOperation {
/// This field is set to `true` after calling `Visitor::visit_callable_decl(...)`
/// if the operation satisfies the characteristics described above.
is_op: bool,
}

impl HirLintPass for Placeholder {
fn check_expr(&self, expr: &qsc_hir::hir::Expr, buffer: &mut Vec<Lint>) {
if let qsc_hir::hir::ExprKind::Lit(Lit::Int(42)) = &expr.kind {
buffer.push(lint!(self, expr.span));
impl IsQuantumOperation {
/// Returns `true` if the declaration is empty.
fn is_empty_decl(spec_decl: Option<&SpecDecl>) -> bool {
match spec_decl {
None => true,
Some(decl) => match &decl.body {
SpecBody::Gen(_) => true,
SpecBody::Impl(_, block) => block.stmts.is_empty(),
},
}
}

/// Returns `true` if the operation is empty.
/// An operation is empty if there is no code for its body and specializations.
fn is_empty_op(call_decl: &CallableDecl) -> bool {
Self::is_empty_decl(Some(&call_decl.body))
&& Self::is_empty_decl(call_decl.adj.as_ref())
&& Self::is_empty_decl(call_decl.ctl.as_ref())
&& Self::is_empty_decl(call_decl.ctl_adj.as_ref())
}
}

impl Visitor<'_> for IsQuantumOperation {
fn visit_callable_decl(&mut self, decl: &CallableDecl) {
if Self::is_empty_op(decl) {
self.is_op = true;
} else {
visit::walk_callable_decl(self, decl);
}
}

fn visit_stmt(&mut self, stmt: &Stmt) {
if !self.is_op {
if let StmtKind::Qubit(..) = &stmt.kind {
self.is_op = true;
} else {
visit::walk_stmt(self, stmt);
}
}
}

fn visit_expr(&mut self, expr: &Expr) {
if !self.is_op {
match &expr.kind {
ExprKind::Call(callee, _) => {
if matches!(&callee.ty, Ty::Arrow(arrow) if arrow.kind == CallableKind::Operation)
{
self.is_op = true;
}
}
ExprKind::Conjugate(..) | ExprKind::Repeat(..) => {
self.is_op = true;
}
_ => {
visit::walk_expr(self, expr);
}
}
}
}
}

/// HIR Lint for [`NeedlessOperation`], suggesting to use function
/// We use [`IsQuantumOperation`] helper to check if a operation has desired operation characteristics
impl HirLintPass for NeedlessOperation {
fn check_callable_decl(&self, decl: &CallableDecl, buffer: &mut Vec<Lint>) {
if decl.kind == CallableKind::Operation {
let mut op_limits = IsQuantumOperation::default();

op_limits.visit_callable_decl(decl);

if !op_limits.is_op {
buffer.push(lint!(self, decl.name.span));
}
}
}
}
157 changes: 135 additions & 22 deletions compiler/qsc_linter/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@ use crate::{
Lint, LintConfig, LintLevel,
};
use expect_test::{expect, Expect};
use indoc::indoc;
use qsc_data_structures::{language_features::LanguageFeatures, target::TargetCapabilityFlags};
use qsc_frontend::compile::{self, CompileUnit, PackageStore, SourceMap};
use qsc_hir::hir::CallableKind;
use qsc_passes::PackageType;

#[test]
fn multiple_lints() {
check(
"let x = ((1 + 2)) / 0;;;;",
&wrap_in_callable("let x = ((1 + 2)) / 0;;;;", CallableKind::Operation),
&expect![[r#"
[
SrcLint {
Expand All @@ -34,6 +36,12 @@ fn multiple_lints() {
message: "unnecessary parentheses",
help: "remove the extra parentheses for clarity",
},
SrcLint {
source: "RunProgram",
level: Warn,
message: "operation does not contain any quantum operations",
help: "this callable can be declared as a function instead",
},
]
"#]],
);
Expand All @@ -42,7 +50,7 @@ fn multiple_lints() {
#[test]
fn double_parens() {
check(
"let x = ((1 + 2));",
&wrap_in_callable("let x = ((1 + 2));", CallableKind::Function),
&expect![[r#"
[
SrcLint {
Expand All @@ -59,7 +67,7 @@ fn double_parens() {
#[test]
fn division_by_zero() {
check(
"let x = 2 / 0;",
&wrap_in_callable("let x = 2 / 0;", CallableKind::Function),
&expect![[r#"
[
SrcLint {
Expand All @@ -76,7 +84,7 @@ fn division_by_zero() {
#[test]
fn needless_parens_in_assignment() {
check(
"let x = (42);",
&wrap_in_callable("let x = (42);", CallableKind::Function),
&expect![[r#"
[
SrcLint {
Expand All @@ -85,12 +93,6 @@ fn needless_parens_in_assignment() {
message: "unnecessary parentheses",
help: "remove the extra parentheses for clarity",
},
SrcLint {
source: "42",
level: Allow,
message: "this a placeholder",
help: "remove after addding the first HIR lint",
},
]
"#]],
);
Expand All @@ -99,7 +101,7 @@ fn needless_parens_in_assignment() {
#[test]
fn needless_parens() {
check(
"let x = (2) + (5 * 4 * (2 ^ 10));",
&wrap_in_callable("let x = (2) + (5 * 4 * (2 ^ 10));", CallableKind::Function),
&expect![[r#"
[
SrcLint {
Expand Down Expand Up @@ -128,7 +130,7 @@ fn needless_parens() {
#[test]
fn redundant_semicolons() {
check(
"let x = 2;;;;;",
&wrap_in_callable("let x = 2;;;;;", CallableKind::Function),
&expect![[r#"
[
SrcLint {
Expand All @@ -143,16 +145,121 @@ fn redundant_semicolons() {
}

#[test]
fn hir_placeholder() {
fn needless_operation_lambda_operations() {
check(
"let placeholder = 42;",
&wrap_in_callable("let a = (a) => a + 1;", CallableKind::Function),
&expect![[r#"
[
SrcLint {
source: "42",
level: Allow,
message: "this a placeholder",
help: "remove after addding the first HIR lint",
source: "",
level: Warn,
message: "operation does not contain any quantum operations",
help: "this callable can be declared as a function instead",
},
]
"#]],
);
}

#[test]
fn needless_operation_no_lint_for_valid_lambda_operations() {
check(
&wrap_in_callable("let op = (q) => H(q);", CallableKind::Function),
&expect![[r"
[]
"]],
);
}

#[test]
fn needless_operation_non_empty_op_and_no_specialization() {
check(
&wrap_in_callable("let x = 2;", CallableKind::Operation),
&expect![[r#"
[
SrcLint {
source: "RunProgram",
level: Warn,
message: "operation does not contain any quantum operations",
help: "this callable can be declared as a function instead",
},
]
"#]],
);
}

#[test]
fn needless_operation_non_empty_op_and_specialization() {
check(
indoc! {"
operation Run(target : Qubit) : Unit is Adj {
body ... {
Message(\"hi\");
}
adjoint self;
}
"},
&expect![[r#"
[
SrcLint {
source: "Run",
level: Warn,
message: "operation does not contain any quantum operations",
help: "this callable can be declared as a function instead",
},
]
"#]],
);
}

#[test]
fn needless_operation_no_lint_for_empty_op_explicit_specialization() {
check(
indoc! {"
operation I(target : Qubit) : Unit {
body ... {}
adjoint self;
}
"},
&expect![[r"
[]
"]],
);
}

#[test]
fn needless_operation_no_lint_for_empty_op_implicit_specialization() {
check(
indoc! {"
operation DoNothing() : Unit is Adj + Ctl {}
"},
&expect![[r"
[]
"]],
);
}

#[test]
fn needless_operation_partial_application() {
check(
indoc! {"
operation PrepareBellState(q1 : Qubit, q2 : Qubit) : Unit {
H(q1);
CNOT(q1, q2);
}
operation PartialApplication(q1 : Qubit) : Qubit => Unit {
return PrepareBellState(q1, _);
}
"},
&expect![[r#"
[
SrcLint {
source: "PartialApplication",
level: Warn,
message: "operation does not contain any quantum operations",
help: "this callable can be declared as a function instead",
},
]
"#]],
Expand Down Expand Up @@ -184,11 +291,17 @@ fn check(source: &str, expected: &Expect) {
/// Wraps some source code into a namespace, to make testing easier.
fn wrap_in_namespace(source: &str) -> String {
format!(
"namespace foo {{
operation RunProgram(vector : Double[]) : Unit {{
"namespace Foo {{
{source}
}}"
)
}

fn wrap_in_callable(source: &str, callable_type: CallableKind) -> String {
format!(
"{callable_type} RunProgram() : Unit {{
{source}
}}
}}"
}}"
)
}

Expand Down
3 changes: 2 additions & 1 deletion language_service/src/code_action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ fn quick_fixes(
kind: Some(CodeActionKind::QuickFix),
is_preferred: None,
}),
LintKind::Ast(AstLint::DivisionByZero) | LintKind::Hir(HirLint::Placeholder) => (),
LintKind::Ast(AstLint::DivisionByZero)
| LintKind::Hir(HirLint::NeedlessOperation) => (),
}
}
}
Expand Down
Loading

0 comments on commit 2454f09

Please sign in to comment.