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

[red-knot] Unresolved imports lint rule #11164

Merged
merged 2 commits into from
Apr 28, 2024
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/red_knot/src/cancellation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ impl CancellationTokenSource {
}
}

#[tracing::instrument(level = "trace")]
#[tracing::instrument(level = "trace", skip_all)]
pub fn cancel(&self) {
let (cancelled, condvar) = &*self.signal;

Expand Down
28 changes: 18 additions & 10 deletions crates/red_knot/src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::path::Path;
use std::sync::Arc;

use crate::files::FileId;
use crate::lint::{Diagnostics, LintSyntaxStorage};
use crate::lint::{Diagnostics, LintSemanticStorage, LintSyntaxStorage};
use crate::module::{Module, ModuleData, ModuleName, ModuleResolver, ModuleSearchPath};
use crate::parse::{Parsed, ParsedStorage};
use crate::source::{Source, SourceStorage};
Expand Down Expand Up @@ -32,13 +32,15 @@ pub trait SemanticDb: SourceDb {

fn symbol_table(&self, file_id: FileId) -> Arc<SymbolTable>;

fn infer_symbol_type(&self, file_id: FileId, symbol_id: SymbolId) -> Type;

fn lint_semantic(&self, file_id: FileId) -> Diagnostics;

// mutations

fn add_module(&mut self, path: &Path) -> Option<(Module, Vec<Arc<ModuleData>>)>;

fn set_module_search_paths(&mut self, paths: Vec<ModuleSearchPath>);

fn infer_symbol_type(&mut self, file_id: FileId, symbol_id: SymbolId) -> Type;
}

pub trait Db: SemanticDb {}
Expand All @@ -55,6 +57,7 @@ pub struct SemanticJar {
pub module_resolver: ModuleResolver,
pub symbol_tables: SymbolTablesStorage,
pub type_store: TypeStore,
pub lint_semantic: LintSemanticStorage,
}

/// Gives access to a specific jar in the database.
Expand All @@ -79,9 +82,12 @@ pub trait HasJar<T> {

#[cfg(test)]
pub(crate) mod tests {
use std::path::Path;
use std::sync::Arc;

use crate::db::{HasJar, SourceDb, SourceJar};
use crate::files::{FileId, Files};
use crate::lint::{lint_syntax, Diagnostics};
use crate::lint::{lint_semantic, lint_syntax, Diagnostics};
use crate::module::{
add_module, file_to_module, path_to_module, resolve_module, set_module_search_paths,
Module, ModuleData, ModuleName, ModuleSearchPath,
Expand All @@ -90,8 +96,6 @@ pub(crate) mod tests {
use crate::source::{source_text, Source};
use crate::symbols::{symbol_table, SymbolId, SymbolTable};
use crate::types::{infer_symbol_type, Type};
use std::path::Path;
use std::sync::Arc;

use super::{SemanticDb, SemanticJar};

Expand Down Expand Up @@ -163,16 +167,20 @@ pub(crate) mod tests {
symbol_table(self, file_id)
}

fn infer_symbol_type(&self, file_id: FileId, symbol_id: SymbolId) -> Type {
infer_symbol_type(self, file_id, symbol_id)
}

fn lint_semantic(&self, file_id: FileId) -> Diagnostics {
lint_semantic(self, file_id)
}

fn add_module(&mut self, path: &Path) -> Option<(Module, Vec<Arc<ModuleData>>)> {
add_module(self, path)
}

fn set_module_search_paths(&mut self, paths: Vec<ModuleSearchPath>) {
set_module_search_paths(self, paths);
}

fn infer_symbol_type(&mut self, file_id: FileId, symbol_id: SymbolId) -> Type {
infer_symbol_type(self, file_id, symbol_id)
}
}
}
7 changes: 7 additions & 0 deletions crates/red_knot/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::fmt::Formatter;
use std::hash::BuildHasherDefault;
use std::ops::Deref;
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -100,3 +101,9 @@ where
Self(value.into())
}
}

impl std::fmt::Display for Name {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
f.write_str(self.as_str())
}
}
133 changes: 130 additions & 3 deletions crates/red_knot/src/lint.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
use std::cell::RefCell;
use std::ops::{Deref, DerefMut};
use std::sync::Arc;

use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::StringLiteral;
use ruff_python_ast::{ModModule, StringLiteral};

use crate::cache::KeyValueCache;
use crate::db::{HasJar, SourceDb, SourceJar};
use crate::db::{HasJar, SemanticDb, SemanticJar, SourceDb, SourceJar};
use crate::files::FileId;
use crate::parse::Parsed;
use crate::source::Source;
use crate::symbols::{Definition, SymbolId, SymbolTable};
use crate::types::Type;

#[tracing::instrument(level = "debug", skip(db))]
pub(crate) fn lint_syntax<Db>(db: &Db, file_id: FileId) -> Diagnostics
Expand Down Expand Up @@ -40,7 +45,7 @@ where
})
}

pub(crate) fn lint_lines(source: &str, diagnostics: &mut Vec<String>) {
fn lint_lines(source: &str, diagnostics: &mut Vec<String>) {
for (line_number, line) in source.lines().enumerate() {
if line.len() < 88 {
continue;
Expand All @@ -57,6 +62,111 @@ pub(crate) fn lint_lines(source: &str, diagnostics: &mut Vec<String>) {
}
}

#[tracing::instrument(level = "debug", skip(db))]
pub(crate) fn lint_semantic<Db>(db: &Db, file_id: FileId) -> Diagnostics
where
Db: SemanticDb + HasJar<SemanticJar>,
{
let storage = &db.jar().lint_semantic;

storage.get(&file_id, |file_id| {
let source = db.source(*file_id);
let parsed = db.parse(*file_id);
let symbols = db.symbol_table(*file_id);

let context = SemanticLintContext {
file_id: *file_id,
source,
parsed,
symbols,
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
db,
diagnostics: RefCell::new(Vec::new()),
};

lint_unresolved_imports(&context);

Diagnostics::from(context.diagnostics.take())
})
}

fn lint_unresolved_imports(context: &SemanticLintContext) {
// TODO: Consider iterating over the dependencies (imports) only instead of all definitions.
for (symbol, definition) in context.symbols().all_definitions() {
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
match definition {
Definition::Import(import) => {
let ty = context.eval_symbol(symbol);

if ty.is_unknown() {
context.push_diagnostic(format!("Unresolved module {}", import.module));
}
}
Definition::ImportFrom(import) => {
let ty = context.eval_symbol(symbol);

if ty.is_unknown() {
let module_name = import.module().map(Deref::deref).unwrap_or_default();
let message = if import.level() > 0 {
format!(
"Unresolved relative import '{}' from {}{}",
import.name(),
".".repeat(import.level() as usize),
module_name
)
} else {
format!(
"Unresolved import '{}' from '{}'",
import.name(),
module_name
)
};

context.push_diagnostic(message);
}
}
_ => {}
}
}
}

pub struct SemanticLintContext<'a> {
file_id: FileId,
source: Source,
parsed: Parsed,
symbols: Arc<SymbolTable>,
db: &'a dyn SemanticDb,
diagnostics: RefCell<Vec<String>>,
}

impl<'a> SemanticLintContext<'a> {
pub fn source_text(&self) -> &str {
self.source.text()
}

pub fn file_id(&self) -> FileId {
self.file_id
}

pub fn ast(&self) -> &ModModule {
self.parsed.ast()
}

pub fn symbols(&self) -> &SymbolTable {
&self.symbols
}

pub fn eval_symbol(&self, symbol_id: SymbolId) -> Type {
self.db.infer_symbol_type(self.file_id, symbol_id)
}

pub fn push_diagnostic(&self, diagnostic: String) {
self.diagnostics.borrow_mut().push(diagnostic);
}

pub fn extend_diagnostics(&mut self, diagnostics: impl IntoIterator<Item = String>) {
self.diagnostics.get_mut().extend(diagnostics);
}
}

#[derive(Debug)]
struct SyntaxLintVisitor<'a> {
diagnostics: Vec<String>,
Expand Down Expand Up @@ -123,3 +233,20 @@ impl DerefMut for LintSyntaxStorage {
&mut self.0
}
}

#[derive(Default, Debug)]
pub struct LintSemanticStorage(KeyValueCache<FileId, Diagnostics>);

impl Deref for LintSemanticStorage {
type Target = KeyValueCache<FileId, Diagnostics>;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl DerefMut for LintSemanticStorage {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
4 changes: 2 additions & 2 deletions crates/red_knot/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Module {
Db: HasJar<SemanticJar>,
{
let (level, module) = match dependency {
Dependency::Module(module) => return Some(ModuleName::new(module)),
Dependency::Module(module) => return Some(module.clone()),
Dependency::Relative { level, module } => (*level, module.as_deref()),
};

Expand Down Expand Up @@ -1012,7 +1012,7 @@ mod tests {
&db,
&Dependency::Relative {
level: NonZeroU32::new(1).unwrap(),
module: None
module: None,
}
)
);
Expand Down
9 changes: 6 additions & 3 deletions crates/red_knot/src/program/check.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
use std::num::NonZeroUsize;

use rayon::max_num_threads;
use rustc_hash::FxHashSet;

use crate::cancellation::CancellationToken;
use crate::db::{SemanticDb, SourceDb};
use crate::files::FileId;
use crate::lint::Diagnostics;
use crate::program::Program;
use crate::symbols::Dependency;
use rayon::max_num_threads;
use rustc_hash::FxHashSet;
use std::num::NonZeroUsize;

impl Program {
/// Checks all open files in the workspace and its dependencies.
Expand Down Expand Up @@ -77,6 +79,7 @@ impl Program {

if self.workspace().is_file_open(file) {
diagnostics.extend_from_slice(&self.lint_syntax(file));
diagnostics.extend_from_slice(&self.lint_semantic(file));
}

Ok(Diagnostics::from(diagnostics))
Expand Down
19 changes: 13 additions & 6 deletions crates/red_knot/src/program/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ use std::sync::Arc;

use crate::db::{Db, HasJar, SemanticDb, SemanticJar, SourceDb, SourceJar};
use crate::files::{FileId, Files};
use crate::lint::{lint_syntax, Diagnostics, LintSyntaxStorage};
use crate::lint::{
lint_semantic, lint_syntax, Diagnostics, LintSemanticStorage, LintSyntaxStorage,
};
use crate::module::{
add_module, file_to_module, path_to_module, resolve_module, set_module_search_paths, Module,
ModuleData, ModuleName, ModuleResolver, ModuleSearchPath,
Expand Down Expand Up @@ -36,6 +38,7 @@ impl Program {
module_resolver: ModuleResolver::new(module_search_paths),
symbol_tables: SymbolTablesStorage::default(),
type_store: TypeStore::default(),
lint_semantic: LintSemanticStorage::default(),
},
files: Files::default(),
workspace,
Expand All @@ -56,6 +59,7 @@ impl Program {
self.source.lint_syntax.remove(&change.id);
// TODO: remove all dependent modules as well
self.semantic.type_store.remove_module(change.id);
self.semantic.lint_semantic.remove(&change.id);
}
}

Expand Down Expand Up @@ -111,19 +115,22 @@ impl SemanticDb for Program {
symbol_table(self, file_id)
}

// Mutations
fn infer_symbol_type(&self, file_id: FileId, symbol_id: SymbolId) -> Type {
infer_symbol_type(self, file_id, symbol_id)
}

fn lint_semantic(&self, file_id: FileId) -> Diagnostics {
lint_semantic(self, file_id)
}

// Mutations
fn add_module(&mut self, path: &Path) -> Option<(Module, Vec<Arc<ModuleData>>)> {
add_module(self, path)
}

fn set_module_search_paths(&mut self, paths: Vec<ModuleSearchPath>) {
set_module_search_paths(self, paths);
}

fn infer_symbol_type(&mut self, file_id: FileId, symbol_id: SymbolId) -> Type {
infer_symbol_type(self, file_id, symbol_id)
}
}

impl Db for Program {}
Expand Down