Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lint groups to Q# #2103

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.

10 changes: 6 additions & 4 deletions compiler/qsc/src/interpret.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,10 +404,12 @@ impl Interpreter {
compile_unit,
// see https://github.com/microsoft/qsharp/pull/1627 for context
// on why we override this config
Some(&[qsc_linter::LintConfig {
kind: LintKind::Hir(HirLint::NeedlessOperation),
level: LintLevel::Warn,
}]),
Some(&[qsc_linter::LintOrGroupConfig::Lint(
qsc_linter::LintConfig {
kind: LintKind::Hir(HirLint::NeedlessOperation),
level: LintLevel::Warn,
},
)]),
)
} else {
Vec::new()
Expand Down
4 changes: 3 additions & 1 deletion compiler/qsc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ pub use qsc_eval::{
};

pub mod linter {
pub use qsc_linter::{run_lints, LintConfig, LintKind, LintLevel};
pub use qsc_linter::{
run_lints, GroupConfig, LintConfig, LintKind, LintLevel, LintOrGroupConfig,
};
}

pub use qsc_doc_gen::{display, generate_docs};
Expand Down
1 change: 1 addition & 0 deletions compiler/qsc_linter/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ qsc_hir = { path = "../qsc_hir" }
qsc_data_structures = { path = "../qsc_data_structures" }
qsc_frontend = { path = "../qsc_frontend" }
qsc_doc_gen = { path = "../qsc_doc_gen" }
rustc-hash = { workspace = true }
serde = { workspace = true }
thiserror = { workspace = true }

Expand Down
4 changes: 3 additions & 1 deletion compiler/qsc_linter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,12 @@

#![deny(missing_docs)]

mod lint_groups;
mod linter;
mod lints;
#[cfg(test)]
mod tests;

pub use linter::{run_lints, Lint, LintConfig, LintKind, LintLevel};
pub use lint_groups::GroupConfig;
pub use linter::{run_lints, Lint, LintConfig, LintKind, LintLevel, LintOrGroupConfig};
pub use lints::{ast::AstLint, hir::HirLint};
42 changes: 42 additions & 0 deletions compiler/qsc_linter/src/lint_groups.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
use serde::{Deserialize, Serialize};

use crate::{LintKind, LintLevel};

/// End-user configuration for a lint group.
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
pub struct GroupConfig {
#[serde(rename = "group")]
/// The lint group.
pub lint_group: LintGroup,
/// The group level.
pub level: LintLevel,
}

#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq)]
#[serde(rename_all = "camelCase")]
pub enum LintGroup {
Pedantic,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "pedantic"?

It has a slightly negative connotation, like "so picky as to be annoying". I do know Clippy uses it, but I think the name makes some sense there: they have many other groups you would want to enable before pedantic. So this is the one that's like a bunch of opinionated lints we didn't know where else to put. Even their documentation hedges it by saying "if you enable this group, expect to also use #[allow] attributes generously"

If this is our only lint group, I don't think "pedantic" makes sense.

My other problem with the name is it doesn't describe a meaningful category or purpose. As a user, how can I tell what I'm getting out of these lints by opting into them?

Do we need groups? If we do, what about more functional categories like "correctness", "style", "deprecations" etc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you here, some extra color to the conversation: I think the pedantic lints in Clippy tend to be much more subjective and not as universally-accepted. I don't think the name pedantic conveys that, but maybe we can come up with a better word. Stylistic? Strict? Opinionated? I like opinionated as a group name.

}

impl LintGroup {
pub fn unfold(self) -> Vec<LintKind> {
use crate::AstLint::*;
use crate::HirLint::*;
match self {
LintGroup::Pedantic => {
vec![
LintKind::Ast(DivisionByZero),
LintKind::Ast(NeedlessParens),
LintKind::Ast(RedundantSemicolons),
LintKind::Ast(DeprecatedNewtype),
LintKind::Ast(DeprecatedSet),
LintKind::Ast(DiscourageChainAssignment),
LintKind::Hir(NeedlessOperation),
LintKind::Hir(DeprecatedFunctionConstructor),
LintKind::Hir(DeprecatedWithOperator),
LintKind::Hir(DeprecatedDoubleColonOperator),
]
}
}
}
}
67 changes: 60 additions & 7 deletions compiler/qsc_linter/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@ pub(crate) mod ast;
pub(crate) mod hir;

use self::{ast::run_ast_lints, hir::run_hir_lints};
use crate::lints::{ast::AstLint, hir::HirLint};
use crate::{
lints::{ast::AstLint, hir::HirLint},
GroupConfig,
};
use miette::{Diagnostic, LabeledSpan};
use qsc_data_structures::span::Span;
use qsc_frontend::compile::{CompileUnit, PackageStore};
use qsc_hir::hir::{Item, ItemId};
use rustc_hash::FxHashMap;
use serde::{Deserialize, Serialize};
use std::fmt::Display;

Expand All @@ -19,22 +23,61 @@ use std::fmt::Display;
pub fn run_lints(
package_store: &PackageStore,
compile_unit: &CompileUnit,
config: Option<&[LintConfig]>,
config: Option<&[LintOrGroupConfig]>,
) -> Vec<Lint> {
let compilation = Compilation {
package_store,
compile_unit,
};

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 unfolded_config = config.map(unfold_groups);

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

let mut lints = Vec::new();
lints.append(&mut ast_lints);
lints.append(&mut hir_lints);
lints
}

/// Unfolds groups into lists of lints. Specific lints override group configs.
pub(crate) fn unfold_groups(config: &[LintOrGroupConfig]) -> Vec<LintConfig> {
let mut config_map: FxHashMap<LintKind, LintLevel> = FxHashMap::default();

// Unfold groups in the order they appear.
for elt in config {
if let LintOrGroupConfig::Group(group) = elt {
for lint in group.lint_group.unfold() {
config_map.insert(lint, group.level);
}
}
}

// Specific lints override group configs.
for elt in config {
if let LintOrGroupConfig::Lint(lint) = elt {
config_map.insert(lint.kind, lint.level);
}
}

config_map
.iter()
.map(|(kind, level)| LintConfig {
kind: *kind,
level: *level,
})
.collect()
}

#[derive(Clone, Copy)]
pub(crate) struct Compilation<'a> {
pub package_store: &'a PackageStore,
Expand Down Expand Up @@ -177,18 +220,28 @@ impl Display for LintLevel {
}
}

/// End-user configuration for each lint level.
/// End-user configuration for a specific lint or a lint group.
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
#[serde(untagged)]
pub enum LintOrGroupConfig {
/// An specific lint configuration.
Lint(LintConfig),
/// A lint group configuration.
Group(GroupConfig),
}

/// End-user configuration for a specific lint.
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
pub struct LintConfig {
#[serde(rename = "lint")]
/// Represents the lint name.
/// The lint name.
pub kind: LintKind,
/// The lint level.
pub level: LintLevel,
}

/// Represents a lint name.
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq)]
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, Hash)]
#[serde(untagged)]
pub enum LintKind {
/// AST lint name.
Expand Down
12 changes: 6 additions & 6 deletions compiler/qsc_linter/src/linter/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use crate::{
lints::ast::{AstLint, CombinedAstLints},
Lint, LintConfig, LintLevel,
Lint, LintLevel,
};
use qsc_ast::{
ast::{
Expand All @@ -24,9 +24,9 @@ pub fn run_ast_lints(
let config: Vec<(AstLint, LintLevel)> = config
.unwrap_or(&[])
.iter()
.filter_map(|lint_config| {
if let LintKind::Ast(kind) = lint_config.kind {
Some((kind, lint_config.level))
.filter_map(|lint| {
if let LintKind::Ast(kind) = lint.kind {
Some((kind, lint.level))
} else {
None
}
Expand Down Expand Up @@ -185,7 +185,7 @@ macro_rules! declare_ast_lints {
use serde::{Deserialize, Serialize};

/// An enum listing all existing AST lints.
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq)]
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, Hash)]
#[serde(rename_all = "camelCase")]
pub enum AstLint {
$(
Expand Down Expand Up @@ -342,4 +342,4 @@ macro_rules! declare_ast_lints {

pub(crate) use declare_ast_lints;

use super::{Compilation, LintKind};
use super::{Compilation, LintConfig, LintKind};
12 changes: 6 additions & 6 deletions compiler/qsc_linter/src/linter/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use crate::{
lints::hir::{CombinedHirLints, HirLint},
Lint, LintConfig, LintLevel,
Lint, LintLevel,
};
use qsc_hir::{
hir::{Block, CallableDecl, Expr, Ident, Item, Package, Pat, QubitInit, SpecDecl, Stmt},
Expand All @@ -21,9 +21,9 @@ pub fn run_hir_lints(
let config: Vec<(HirLint, LintLevel)> = config
.unwrap_or(&[])
.iter()
.filter_map(|lint_config| {
if let LintKind::Hir(kind) = lint_config.kind {
Some((kind, lint_config.level))
.filter_map(|lint| {
if let LintKind::Hir(kind) = lint.kind {
Some((kind, lint.level))
} else {
None
}
Expand Down Expand Up @@ -149,7 +149,7 @@ macro_rules! declare_hir_lints {
use serde::{Deserialize, Serialize};

/// An enum listing all existing HIR lints.
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq)]
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, Hash)]
#[serde(rename_all = "camelCase")]
pub enum HirLint {
$(
Expand Down Expand Up @@ -264,4 +264,4 @@ macro_rules! declare_hir_lints {

pub(crate) use declare_hir_lints;

use super::{Compilation, LintKind};
use super::{Compilation, LintConfig, LintKind};
Loading
Loading