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 rules table to configuration #15645

Merged
merged 2 commits into from
Jan 23, 2025
Merged
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
91 changes: 91 additions & 0 deletions crates/red_knot/tests/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,97 @@ stat = add(10, 15)
Ok(())
}

/// The rule severity can be changed in the configuration file
#[test]
fn rule_severity() -> anyhow::Result<()> {
let tempdir = TempDir::new()?;

let project_dir = tempdir.path().canonicalize()?;

std::fs::write(
project_dir.join("test.py"),
r#"
y = 4 / 0

for a in range(0, y):
x = a

print(x) # possibly-unresolved-reference
"#,
)
.context("Failed to write `test.py`")?;

// Assert that there's a possibly unresolved reference diagnostic
// and that division-by-zero has a severity of error by default.
insta::with_settings!({filters => vec![(&*tempdir_filter(&project_dir), "<temp_dir>/")]}, {
assert_cmd_snapshot!(knot().current_dir(&project_dir), @r"
success: false
exit_code: 1
----- stdout -----
error[lint:division-by-zero] <temp_dir>/test.py:2:5 Cannot divide object of type `Literal[4]` by zero
warning[lint:possibly-unresolved-reference] <temp_dir>/test.py:7:7 Name `x` used when possibly not defined

----- stderr -----
");
});

std::fs::write(
project_dir.join("pyproject.toml"),
r#"
[tool.knot.rules]
division-by-zero = "warn" # demote to warn
possibly-unresolved-reference = "ignore"
"#,
)
.context("Failed to write `pyproject.toml`")?;

insta::with_settings!({filters => vec![(&*tempdir_filter(&project_dir), "<temp_dir>/")]}, {
assert_cmd_snapshot!(knot().current_dir(project_dir), @r"
success: false
exit_code: 1
----- stdout -----
warning[lint:division-by-zero] <temp_dir>/test.py:2:5 Cannot divide object of type `Literal[4]` by zero

----- stderr -----
");
});

Ok(())
}

/// Red Knot warns about unknown rules
#[test]
fn unknown_rules() -> anyhow::Result<()> {
let tempdir = TempDir::new()?;

let project_dir = tempdir.path().canonicalize()?;

std::fs::write(
project_dir.join("pyproject.toml"),
r#"
[tool.knot.rules]
division-by-zer = "warn" # incorrect rule name
"#,
)
.context("Failed to write `pyproject.toml`")?;

std::fs::write(project_dir.join("test.py"), r#"print(10)"#)
.context("Failed to write `test.py`")?;

insta::with_settings!({filters => vec![(&*tempdir_filter(&project_dir), "<temp_dir>/")]}, {
assert_cmd_snapshot!(knot().current_dir(project_dir), @r"
success: false
exit_code: 1
----- stdout -----
warning[unknown-rule] Unknown lint rule `division-by-zer`
carljm marked this conversation as resolved.
Show resolved Hide resolved

----- stderr -----
");
});

Ok(())
}

fn knot() -> Command {
Command::new(get_cargo_bin("red_knot"))
}
Expand Down
10 changes: 3 additions & 7 deletions crates/red_knot_project/src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::panic::RefUnwindSafe;
use std::sync::Arc;

use crate::DEFAULT_LINT_REGISTRY;
use crate::{check_file, Project, ProjectMetadata};
use crate::{Project, ProjectMetadata};
use red_knot_python_semantic::lint::{LintRegistry, RuleSelection};
use red_knot_python_semantic::{Db as SemanticDb, Program};
use ruff_db::diagnostic::Diagnostic;
Expand All @@ -27,22 +27,18 @@ pub struct ProjectDatabase {
storage: salsa::Storage<ProjectDatabase>,
files: Files,
system: Arc<dyn System + Send + Sync + RefUnwindSafe>,
rule_selection: Arc<RuleSelection>,
}

impl ProjectDatabase {
pub fn new<S>(project_metadata: ProjectMetadata, system: S) -> anyhow::Result<Self>
where
S: System + 'static + Send + Sync + RefUnwindSafe,
{
let rule_selection = RuleSelection::from_registry(&DEFAULT_LINT_REGISTRY);

let mut db = Self {
project: None,
storage: salsa::Storage::default(),
files: Files::default(),
system: Arc::new(system),
rule_selection: Arc::new(rule_selection),
};

// TODO: Use the `program_settings` to compute the key for the database's persistent
Expand All @@ -66,7 +62,7 @@ impl ProjectDatabase {
pub fn check_file(&self, file: File) -> Result<Vec<Box<dyn Diagnostic>>, Cancelled> {
let _span = tracing::debug_span!("check_file", file=%file.path(self)).entered();

self.with_db(|db| check_file(db, file))
self.with_db(|db| self.project().check_file(db, file))
}

/// Returns a mutable reference to the system.
Expand Down Expand Up @@ -119,7 +115,7 @@ impl SemanticDb for ProjectDatabase {
}

fn rule_selection(&self) -> &RuleSelection {
&self.rule_selection
self.project().rule_selection(self)
}

fn lint_registry(&self) -> &LintRegistry {
Expand Down
64 changes: 52 additions & 12 deletions crates/red_knot_project/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
#![allow(clippy::ref_option)]

use red_knot_python_semantic::lint::{LintRegistry, LintRegistryBuilder};
use crate::metadata::options::OptionDiagnostic;
pub use db::{Db, ProjectDatabase};
use files::{Index, Indexed, IndexedFiles};
pub use metadata::{ProjectDiscoveryError, ProjectMetadata};
use red_knot_python_semantic::lint::{LintRegistry, LintRegistryBuilder, RuleSelection};
use red_knot_python_semantic::register_lints;
use red_knot_python_semantic::types::check_types;
use ruff_db::diagnostic::{Diagnostic, DiagnosticId, ParseDiagnostic, Severity};
Expand All @@ -17,10 +21,6 @@ use salsa::Setter;
use std::borrow::Cow;
use std::sync::Arc;

pub use db::{Db, ProjectDatabase};
use files::{Index, Indexed, IndexedFiles};
pub use metadata::{ProjectDiscoveryError, ProjectMetadata};

pub mod combine;

mod db;
Expand Down Expand Up @@ -68,6 +68,7 @@ pub struct Project {
pub metadata: ProjectMetadata,
}

#[salsa::tracked]
impl Project {
pub fn from_metadata(db: &dyn Db, metadata: ProjectMetadata) -> Self {
Project::builder(metadata)
Expand Down Expand Up @@ -96,13 +97,34 @@ impl Project {
self.reload_files(db);
}

pub fn rule_selection(self, db: &dyn Db) -> &RuleSelection {
let (selection, _) = self.rule_selection_with_diagnostics(db);
selection
}

#[salsa::tracked(return_ref)]
fn rule_selection_with_diagnostics(
self,
db: &dyn Db,
) -> (RuleSelection, Vec<OptionDiagnostic>) {
self.metadata(db).options().to_rule_selection(db)
}

/// Checks all open files in the project and its dependencies.
pub fn check(self, db: &ProjectDatabase) -> Vec<Box<dyn Diagnostic>> {
pub(crate) fn check(self, db: &ProjectDatabase) -> Vec<Box<dyn Diagnostic>> {
let project_span = tracing::debug_span!("Project::check");
let _span = project_span.enter();

tracing::debug!("Checking project '{name}'", name = self.name(db));
let result = Arc::new(std::sync::Mutex::new(Vec::new()));

let mut diagnostics: Vec<Box<dyn Diagnostic>> = Vec::new();
let (_, options_diagnostics) = self.rule_selection_with_diagnostics(db);
diagnostics.extend(options_diagnostics.iter().map(|diagnostic| {
let diagnostic: Box<dyn Diagnostic> = Box::new(diagnostic.clone());
diagnostic
}));

let result = Arc::new(std::sync::Mutex::new(diagnostics));
let inner_result = Arc::clone(&result);

let db = db.clone();
Expand All @@ -119,7 +141,7 @@ impl Project {
let check_file_span = tracing::debug_span!(parent: &project_span, "check_file", file=%file.path(&db));
let _entered = check_file_span.entered();

let file_diagnostics = check_file(&db, file);
let file_diagnostics = check_file_impl(&db, file);
result.lock().unwrap().extend(file_diagnostics);
});
}
Expand All @@ -128,6 +150,23 @@ impl Project {
Arc::into_inner(result).unwrap().into_inner().unwrap()
}

pub(crate) fn check_file(self, db: &dyn Db, file: File) -> Vec<Box<dyn Diagnostic>> {
Copy link
Member Author

Choose a reason for hiding this comment

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

I included the rule diagnostics in check_file because an API consumer (e.g., the LSP) can decide only to call check_file but never project.check and would never see the diagnostics.

The counter-argument is that these diagnostics aren't related to file so they shouldn't be shown.

I'm not sure what the right answer is, but I defaulted to including them for now.

let (_, options_diagnostics) = self.rule_selection_with_diagnostics(db);

let mut file_diagnostics: Vec<_> = options_diagnostics
.iter()
.map(|diagnostic| {
let diagnostic: Box<dyn Diagnostic> = Box::new(diagnostic.clone());
diagnostic
})
.collect();

let check_diagnostics = check_file_impl(db, file);
file_diagnostics.extend(check_diagnostics);

file_diagnostics
}

/// Opens a file in the project.
///
/// This changes the behavior of `check` to only check the open files rather than all files in the project.
Expand Down Expand Up @@ -265,8 +304,9 @@ impl Project {
}
}

pub(crate) fn check_file(db: &dyn Db, file: File) -> Vec<Box<dyn Diagnostic>> {
fn check_file_impl(db: &dyn Db, file: File) -> Vec<Box<dyn Diagnostic>> {
let mut diagnostics: Vec<Box<dyn Diagnostic>> = Vec::new();

// Abort checking if there are IO errors.
let source = source_text(db.upcast(), file);

Expand Down Expand Up @@ -418,7 +458,7 @@ impl Diagnostic for IOErrorDiagnostic {
#[cfg(test)]
mod tests {
use crate::db::tests::TestDb;
use crate::{check_file, ProjectMetadata};
use crate::{check_file_impl, ProjectMetadata};
use red_knot_python_semantic::types::check_types;
use ruff_db::diagnostic::Diagnostic;
use ruff_db::files::system_path_to_file;
Expand All @@ -442,7 +482,7 @@ mod tests {

assert_eq!(source_text(&db, file).as_str(), "");
assert_eq!(
check_file(&db, file)
check_file_impl(&db, file)
.into_iter()
.map(|diagnostic| diagnostic.message().into_owned())
.collect::<Vec<_>>(),
Expand All @@ -458,7 +498,7 @@ mod tests {

assert_eq!(source_text(&db, file).as_str(), "");
assert_eq!(
check_file(&db, file)
check_file_impl(&db, file)
.into_iter()
.map(|diagnostic| diagnostic.message().into_owned())
.collect::<Vec<_>>(),
Expand Down
Loading
Loading