Skip to content

Commit

Permalink
lint and code action
Browse files Browse the repository at this point in the history
  • Loading branch information
@ScottCarda-MS
ScottCarda-MS committed Dec 12, 2024
1 parent 33dfa81 commit a474203
Show file tree
Hide file tree
Showing 5 changed files with 162 additions and 90 deletions.
2 changes: 1 addition & 1 deletion compiler/qsc_linter/src/linter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub fn run_lints(
compile_unit,
};

let mut ast_lints = run_ast_lints(&compile_unit.ast.package, config);
let mut ast_lints = run_ast_lints(&compile_unit.ast.package, config, compilation);
let mut hir_lints = run_hir_lints(&compile_unit.package, config, compilation);

let mut lints = Vec::new();
Expand Down
161 changes: 102 additions & 59 deletions compiler/qsc_linter/src/linter/ast.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ use qsc_ast::{
/// The entry point to the AST linter. It takes a [`qsc_ast::ast::Package`]
/// as input and outputs a [`Vec<Lint>`](Lint).
#[must_use]
pub fn run_ast_lints(package: &qsc_ast::ast::Package, config: Option<&[LintConfig]>) -> Vec<Lint> {
pub fn run_ast_lints(
package: &qsc_ast::ast::Package,
config: Option<&[LintConfig]>,
compilation: Compilation,
) -> Vec<Lint> {
let config: Vec<(AstLint, LintLevel)> = config
.unwrap_or(&[])
.iter()
Expand All @@ -29,7 +33,7 @@ pub fn run_ast_lints(package: &qsc_ast::ast::Package, config: Option<&[LintConfi
})
.collect();

let mut lints = CombinedAstLints::from_config(config);
let mut lints = CombinedAstLints::from_config(config, compilation);

for node in &package.nodes {
match node {
Expand All @@ -46,23 +50,65 @@ pub fn run_ast_lints(package: &qsc_ast::ast::Package, config: Option<&[LintConfi
/// The trait provides default empty implementations for the rest of the methods,
/// which will be optimized to a no-op by the rust compiler.
pub(crate) trait AstLintPass {
fn check_attr(&self, _attr: &Attr, _buffer: &mut Vec<Lint>) {}
fn check_block(&self, _block: &Block, _buffer: &mut Vec<Lint>) {}
fn check_callable_decl(&self, _callable_decl: &CallableDecl, _buffer: &mut Vec<Lint>) {}
fn check_expr(&self, _expr: &Expr, _buffer: &mut Vec<Lint>) {}
fn check_functor_expr(&self, _functor_expr: &FunctorExpr, _buffer: &mut Vec<Lint>) {}
fn check_ident(&self, _ident: &Ident, _buffer: &mut Vec<Lint>) {}
fn check_item(&self, _item: &Item, _buffer: &mut Vec<Lint>) {}
fn check_namespace(&self, _namespace: &Namespace, _buffer: &mut Vec<Lint>) {}
fn check_package(&self, _package: &Package, _buffer: &mut Vec<Lint>) {}
fn check_pat(&self, _pat: &Pat, _buffer: &mut Vec<Lint>) {}
fn check_path(&self, _path: &Path, _buffer: &mut Vec<Lint>) {}
fn check_path_kind(&self, _path: &PathKind, _buffer: &mut Vec<Lint>) {}
fn check_qubit_init(&self, _qubit_init: &QubitInit, _buffer: &mut Vec<Lint>) {}
fn check_spec_decl(&self, _spec_decl: &SpecDecl, _buffer: &mut Vec<Lint>) {}
fn check_stmt(&self, _stmt: &Stmt, _buffer: &mut Vec<Lint>) {}
fn check_ty(&self, _ty: &Ty, _buffer: &mut Vec<Lint>) {}
fn check_ty_def(&self, _ty_def: &TyDef, _buffer: &mut Vec<Lint>) {}
fn check_attr(&self, _attr: &Attr, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
fn check_block(&self, _block: &Block, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
fn check_callable_decl(
&self,
_callable_decl: &CallableDecl,
_buffer: &mut Vec<Lint>,
_compilation: Compilation,
) {
}
fn check_expr(&self, _expr: &Expr, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
fn check_functor_expr(
&self,
_functor_expr: &FunctorExpr,
_buffer: &mut Vec<Lint>,
_compilation: Compilation,
) {
}
fn check_ident(&self, _ident: &Ident, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
fn check_item(&self, _item: &Item, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
fn check_namespace(
&self,
_namespace: &Namespace,
_buffer: &mut Vec<Lint>,
_compilation: Compilation,
) {
}
fn check_package(
&self,
_package: &Package,
_buffer: &mut Vec<Lint>,
_compilation: Compilation,
) {
}
fn check_pat(&self, _pat: &Pat, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
fn check_path(&self, _path: &Path, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
fn check_path_kind(
&self,
_path: &PathKind,
_buffer: &mut Vec<Lint>,
_compilation: Compilation,
) {
}
fn check_qubit_init(
&self,
_qubit_init: &QubitInit,
_buffer: &mut Vec<Lint>,
_compilation: Compilation,
) {
}
fn check_spec_decl(
&self,
_spec_decl: &SpecDecl,
_buffer: &mut Vec<Lint>,
_compilation: Compilation,
) {
}
fn check_stmt(&self, _stmt: &Stmt, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
fn check_ty(&self, _ty: &Ty, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
fn check_ty_def(&self, _ty_def: &TyDef, _buffer: &mut Vec<Lint>, _compilation: Compilation) {}
}

/// This macro allow us to declare lints while avoiding boilerplate. It does three things:
Expand All @@ -82,7 +128,7 @@ macro_rules! declare_ast_lints {
// This is a silly wrapper module to avoid contaminating the environment
// calling the macro with unwanted imports.
mod _ast_macro_expansion {
use crate::{linter::ast::{declare_ast_lints, AstLintPass}, Lint, LintLevel};
use crate::{linter::Compilation, linter::ast::{declare_ast_lints, AstLintPass}, Lint, LintLevel};
use qsc_ast::{
ast::{
Attr, Block, CallableDecl, Expr, FunctorExpr, Ident, Item, Namespace, Package, Pat, Path, PathKind,
Expand All @@ -106,25 +152,20 @@ macro_rules! declare_ast_lints {

// Declare & implement a struct representing a lint.
(@LINT_STRUCT $lint_name:ident, $default_level:expr, $msg:expr, $help:expr) => {

#[derive(Default)]
pub(crate) struct $lint_name {
level: LintLevel,
}

impl Default for $lint_name {
fn default() -> Self {
Self { level: Self::DEFAULT_LEVEL }
}
}

impl From<LintLevel> for $lint_name {
fn from(value: LintLevel) -> Self {
Self { level: value }
}
}

impl $lint_name {
const DEFAULT_LEVEL: LintLevel = $default_level;

fn new() -> Self {
#[allow(clippy::needless_update)]
Self { level: Self::DEFAULT_LEVEL, ..Default::default() }
}

const fn lint_kind(&self) -> LintKind {
LintKind::Ast(AstLint::$lint_name)
}
Expand Down Expand Up @@ -164,24 +205,26 @@ macro_rules! declare_ast_lints {
/// Combined AST lints for speed. This combined lint allow us to
/// evaluate all the lints in a single AST pass, instead of doing
/// an individual pass for each lint in the linter.
pub(crate) struct CombinedAstLints {
pub(crate) struct CombinedAstLints<'compilation> {
pub buffer: Vec<Lint>,
pub compilation: Compilation<'compilation>,
$($lint_name: $lint_name),*
}

impl Default for CombinedAstLints {
fn default() -> Self {
impl<'compilation> CombinedAstLints<'compilation> {
fn new(compilation: Compilation<'compilation>) -> Self {
Self {
buffer: Vec::default(),
$($lint_name: <$lint_name>::default()),*
buffer: Vec::new(),
compilation,
$($lint_name: <$lint_name>::new()),*
}
}
}

// Most of the calls here are empty methods and they get optimized at compile time to a no-op.
impl CombinedAstLints {
pub fn from_config(config: Vec<(AstLint, LintLevel)>) -> Self {
let mut combined_ast_lints = Self::default();
impl<'compilation> CombinedAstLints<'compilation> {
pub fn from_config(config: Vec<(AstLint, LintLevel)>, compilation: Compilation<'compilation>) -> Self {
let mut combined_ast_lints = Self::new(compilation);
for (lint, level) in config {
match lint {
$(AstLint::$lint_name => combined_ast_lints.$lint_name.level = level),*
Expand All @@ -190,26 +233,26 @@ macro_rules! declare_ast_lints {
combined_ast_lints
}

fn check_package(&mut self, package: &Package) { $(self.$lint_name.check_package(package, &mut self.buffer));*; }
fn check_namespace(&mut self, namespace: &Namespace) { $(self.$lint_name.check_namespace(namespace, &mut self.buffer));*; }
fn check_item(&mut self, item: &Item) { $(self.$lint_name.check_item(item, &mut self.buffer));*; }
fn check_attr(&mut self, attr: &Attr) { $(self.$lint_name.check_attr(attr, &mut self.buffer));*; }
fn check_ty_def(&mut self, def: &TyDef) { $(self.$lint_name.check_ty_def(def, &mut self.buffer));*; }
fn check_callable_decl(&mut self, decl: &CallableDecl) { $(self.$lint_name.check_callable_decl(decl, &mut self.buffer));*; }
fn check_spec_decl(&mut self, decl: &SpecDecl) { $(self.$lint_name.check_spec_decl(decl, &mut self.buffer));*; }
fn check_functor_expr(&mut self, expr: &FunctorExpr) { $(self.$lint_name.check_functor_expr(expr, &mut self.buffer));*; }
fn check_ty(&mut self, ty: &Ty) { $(self.$lint_name.check_ty(ty, &mut self.buffer));*; }
fn check_block(&mut self, block: &Block) { $(self.$lint_name.check_block(block, &mut self.buffer));*; }
fn check_stmt(&mut self, stmt: &Stmt) { $(self.$lint_name.check_stmt(stmt, &mut self.buffer));*; }
fn check_expr(&mut self, expr: &Expr) { $(self.$lint_name.check_expr(expr, &mut self.buffer));*; }
fn check_pat(&mut self, pat: &Pat) { $(self.$lint_name.check_pat(pat, &mut self.buffer));*; }
fn check_qubit_init(&mut self, init: &QubitInit) { $(self.$lint_name.check_qubit_init(init, &mut self.buffer));*; }
fn check_path(&mut self, path: &Path) { $(self.$lint_name.check_path(path, &mut self.buffer));*; }
fn check_path_kind(&mut self, path: &PathKind) { $(self.$lint_name.check_path_kind(path, &mut self.buffer));*; }
fn check_ident(&mut self, ident: &Ident) { $(self.$lint_name.check_ident(ident, &mut self.buffer));*; }
fn check_package(&mut self, package: &Package) { $(self.$lint_name.check_package(package, &mut self.buffer, self.compilation));*; }
fn check_namespace(&mut self, namespace: &Namespace) { $(self.$lint_name.check_namespace(namespace, &mut self.buffer, self.compilation));*; }
fn check_item(&mut self, item: &Item) { $(self.$lint_name.check_item(item, &mut self.buffer, self.compilation));*; }
fn check_attr(&mut self, attr: &Attr) { $(self.$lint_name.check_attr(attr, &mut self.buffer, self.compilation));*; }
fn check_ty_def(&mut self, def: &TyDef) { $(self.$lint_name.check_ty_def(def, &mut self.buffer, self.compilation));*; }
fn check_callable_decl(&mut self, decl: &CallableDecl) { $(self.$lint_name.check_callable_decl(decl, &mut self.buffer, self.compilation));*; }
fn check_spec_decl(&mut self, decl: &SpecDecl) { $(self.$lint_name.check_spec_decl(decl, &mut self.buffer, self.compilation));*; }
fn check_functor_expr(&mut self, expr: &FunctorExpr) { $(self.$lint_name.check_functor_expr(expr, &mut self.buffer, self.compilation));*; }
fn check_ty(&mut self, ty: &Ty) { $(self.$lint_name.check_ty(ty, &mut self.buffer, self.compilation));*; }
fn check_block(&mut self, block: &Block) { $(self.$lint_name.check_block(block, &mut self.buffer, self.compilation));*; }
fn check_stmt(&mut self, stmt: &Stmt) { $(self.$lint_name.check_stmt(stmt, &mut self.buffer, self.compilation));*; }
fn check_expr(&mut self, expr: &Expr) { $(self.$lint_name.check_expr(expr, &mut self.buffer, self.compilation));*; }
fn check_pat(&mut self, pat: &Pat) { $(self.$lint_name.check_pat(pat, &mut self.buffer, self.compilation));*; }
fn check_qubit_init(&mut self, init: &QubitInit) { $(self.$lint_name.check_qubit_init(init, &mut self.buffer, self.compilation));*; }
fn check_path(&mut self, path: &Path) { $(self.$lint_name.check_path(path, &mut self.buffer, self.compilation));*; }
fn check_path_kind(&mut self, path: &PathKind) { $(self.$lint_name.check_path_kind(path, &mut self.buffer, self.compilation));*; }
fn check_ident(&mut self, ident: &Ident) { $(self.$lint_name.check_ident(ident, &mut self.buffer, self.compilation));*; }
}

impl<'a> Visitor<'a> for CombinedAstLints {
impl<'a> Visitor<'a> for CombinedAstLints<'_> {
fn visit_package(&mut self, package: &'a Package) {
self.check_package(package);
visit::walk_package(self, package);
Expand Down Expand Up @@ -299,4 +342,4 @@ macro_rules! declare_ast_lints {

pub(crate) use declare_ast_lints;

use super::LintKind;
use super::{Compilation, LintKind};
38 changes: 32 additions & 6 deletions compiler/qsc_linter/src/lints/ast.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Licensed under the MIT License.

use super::lint;
use crate::linter::ast::declare_ast_lints;
use crate::linter::{ast::declare_ast_lints, Compilation};
use qsc_ast::ast::{BinOp, Block, Expr, ExprKind, Item, ItemKind, Lit, Stmt, StmtKind};
use qsc_data_structures::span::Span;

Expand All @@ -28,10 +28,11 @@ declare_ast_lints! {
(NeedlessParens, LintLevel::Allow, "unnecessary parentheses", "remove the extra parentheses for clarity"),
(RedundantSemicolons, LintLevel::Warn, "redundant semicolons", "remove the redundant semicolons"),
(DeprecatedNewtype, LintLevel::Allow, "deprecated `newtype` declarations", "`newtype` declarations are deprecated, use `struct` instead"),
(DeprecatedSet, LintLevel::Warn, "deprecated use of `set` keyword", "`set` keywords are deprecated for assignments, they can be removed"),
}

impl AstLintPass for DivisionByZero {
fn check_expr(&self, expr: &Expr, buffer: &mut Vec<Lint>) {
fn check_expr(&self, expr: &Expr, buffer: &mut Vec<Lint>, _compilation: Compilation) {
if let ExprKind::BinOp(BinOp::Div, _, ref rhs) = *expr.kind {
if let ExprKind::Lit(ref lit) = *rhs.kind {
if let Lit::Int(0) = **lit {
Expand Down Expand Up @@ -82,7 +83,7 @@ impl NeedlessParens {
}

impl AstLintPass for NeedlessParens {
fn check_expr(&self, expr: &Expr, buffer: &mut Vec<Lint>) {
fn check_expr(&self, expr: &Expr, buffer: &mut Vec<Lint>, _compilation: Compilation) {
match &*expr.kind {
ExprKind::BinOp(_, left, right) => {
self.push(expr, left, buffer);
Expand All @@ -96,7 +97,7 @@ impl AstLintPass for NeedlessParens {
}

/// Checks the assignment statements.
fn check_stmt(&self, stmt: &Stmt, buffer: &mut Vec<Lint>) {
fn check_stmt(&self, stmt: &Stmt, buffer: &mut Vec<Lint>, _compilation: Compilation) {
if let StmtKind::Local(_, _, right) = &*stmt.kind {
if let ExprKind::Paren(_) = &*right.kind {
buffer.push(lint!(
Expand Down Expand Up @@ -124,7 +125,7 @@ impl AstLintPass for RedundantSemicolons {
/// semicolon is parsed as an Empty statement. If we have multiple empty
/// statements in a row, we group them as single lint, that spans from
/// the first redundant semicolon to the last redundant semicolon.
fn check_block(&self, block: &Block, buffer: &mut Vec<Lint>) {
fn check_block(&self, block: &Block, buffer: &mut Vec<Lint>, _compilation: Compilation) {
// a finite state machine that keeps track of the span of the redundant semicolons
// None: no redundant semicolons
// Some(_): one or more redundant semicolons
Expand Down Expand Up @@ -166,9 +167,34 @@ fn precedence(expr: &Expr) -> u8 {

/// Creates a lint for deprecated user-defined types declarations using `newtype`.
impl AstLintPass for DeprecatedNewtype {
fn check_item(&self, item: &Item, buffer: &mut Vec<Lint>) {
fn check_item(&self, item: &Item, buffer: &mut Vec<Lint>, _compilation: Compilation) {
if let ItemKind::Ty(_, _) = item.kind.as_ref() {
buffer.push(lint!(self, item.span));
}
}
}

impl AstLintPass for DeprecatedSet {
fn check_expr(&self, expr: &Expr, buffer: &mut Vec<Lint>, compilation: Compilation) {
// If the expression is an assignment, check if the `set` keyword is used.
match expr.kind.as_ref() {
ExprKind::Assign(_, _)
| ExprKind::AssignOp(_, _, _)
| ExprKind::AssignUpdate(_, _, _) => {
if compilation.get_source_code(expr.span).starts_with("set ") {
// If the `set` keyword is used, push a lint.
let span = Span {
lo: expr.span.lo,
hi: expr.span.lo + 3,
};
let code_action_span = Span {
lo: span.lo,
hi: span.lo + 4,
};
buffer.push(lint!(self, span, vec![(String::new(), code_action_span)]));
}
}
_ => {}
}
}
}
28 changes: 27 additions & 1 deletion compiler/qsc_linter/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,32 @@ use qsc_frontend::compile::{self, CompileUnit, PackageStore, SourceMap};
use qsc_hir::hir::CallableKind;
use qsc_passes::PackageType;

#[test]
fn set_keyword_lint() {
check(
&wrap_in_callable("set x = 3;", CallableKind::Function),
&expect![[r#"
[
SrcLint {
source: "set",
level: Warn,
message: "deprecated use of `set` keyword",
help: "`set` keywords are deprecated for assignments, they can be removed",
code_action_edits: [
(
"",
Span {
lo: 71,
hi: 75,
},
),
],
},
]
"#]],
);
}

#[test]
fn multiple_lints() {
check(
Expand Down Expand Up @@ -677,7 +703,7 @@ fn run_lints(
compile_unit,
};

let mut ast_lints = run_ast_lints(&compile_unit.ast.package, config);
let mut ast_lints = run_ast_lints(&compile_unit.ast.package, config, compilation);
let mut hir_lints = run_hir_lints(&compile_unit.package, config, compilation);
let mut lints = Vec::new();
lints.append(&mut ast_lints);
Expand Down
23 changes: 0 additions & 23 deletions compiler/qsc_parse/src/expr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,29 +343,6 @@ fn expr_if(s: &mut ParserContext) -> Result<Box<ExprKind>> {
Ok(Box::new(ExprKind::If(cond, body, otherwise)))
}

fn expr_set(s: &mut ParserContext) -> Result<Box<ExprKind>> {
let lhs = expr(s)?;
if token(s, TokenKind::Eq).is_ok() {
let rhs = expr(s)?;
Ok(Box::new(ExprKind::Assign(lhs, rhs)))
} else if token(s, TokenKind::WSlashEq).is_ok() {
let index = expr(s)?;
token(s, TokenKind::LArrow)?;
let rhs = expr(s)?;
Ok(Box::new(ExprKind::AssignUpdate(lhs, index, rhs)))
} else if let TokenKind::BinOpEq(op) = s.peek().kind {
s.advance();
let rhs = expr(s)?;
Ok(Box::new(ExprKind::AssignOp(closed_bin_op(op), lhs, rhs)))
} else {
Err(Error::new(ErrorKind::Rule(
"assignment operator",
s.peek().kind,
s.peek().span,
)))
}
}

fn expr_array(s: &mut ParserContext) -> Result<Box<ExprKind>> {
token(s, TokenKind::Open(Delim::Bracket))?;
let kind = expr_array_core(s)?;
Expand Down

0 comments on commit a474203

Please sign in to comment.