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

Use a custom error type for symbol-import results #4688

Merged
merged 2 commits into from
May 30, 2023
Merged
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
66 changes: 45 additions & 21 deletions crates/ruff/src/importer/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
//! Add and modify import statements to make module members available during fix execution.

use anyhow::{bail, Result};
use std::error::Error;

use anyhow::Result;
use libcst_native::{Codegen, CodegenState, ImportAlias, Name, NameOrAttribute};
use ruff_text_size::TextSize;
use rustpython_parser::ast::{self, Ranged, Stmt, Suite};
Expand Down Expand Up @@ -69,16 +71,10 @@ impl<'a> Importer<'a> {
member: &str,
at: TextSize,
semantic_model: &SemanticModel,
) -> Result<(Edit, String)> {
) -> Result<(Edit, String), ResolutionError> {
match self.get_symbol(module, member, at, semantic_model) {
Some(result) => result,
None => self.import_symbol(module, member, at, semantic_model),
Some(Resolution::Success(edit, binding)) => Ok((edit, binding)),
Some(Resolution::LateBinding) => {
bail!("Unable to use existing symbol due to late binding")
}
Some(Resolution::IncompatibleContext) => {
bail!("Unable to use existing symbol due to incompatible context")
}
}
}

Expand All @@ -89,7 +85,7 @@ impl<'a> Importer<'a> {
member: &str,
at: TextSize,
semantic_model: &SemanticModel,
) -> Option<Resolution> {
) -> Option<Result<(Edit, String), ResolutionError>> {
// If the symbol is already available in the current scope, use it.
let imported_name = semantic_model.resolve_qualified_import_name(module, member)?;

Expand All @@ -101,13 +97,13 @@ impl<'a> Importer<'a> {
// unclear whether should add an import statement at the top of the file, since it could
// be shadowed between the import and the current location.
if imported_name.range().start() > at {
return Some(Resolution::LateBinding);
return Some(Err(ResolutionError::ImportAfterUsage));
}

// If the symbol source (i.e., the import statement) is in a typing-only context, but we're
// in a runtime context, abort.
if imported_name.context().is_typing() && semantic_model.execution_context().is_runtime() {
return Some(Resolution::IncompatibleContext);
return Some(Err(ResolutionError::IncompatibleContext));
}

// We also add a no-op edit to force conflicts with any other fixes that might try to
Expand All @@ -130,7 +126,7 @@ impl<'a> Importer<'a> {
self.locator.slice(imported_name.range()).to_string(),
imported_name.range(),
);
Some(Resolution::Success(import_edit, imported_name.into_name()))
Some(Ok((import_edit, imported_name.into_name())))
}

/// Generate an [`Edit`] to reference the given symbol. Returns the [`Edit`] necessary to make
Expand All @@ -145,16 +141,18 @@ impl<'a> Importer<'a> {
member: &str,
at: TextSize,
semantic_model: &SemanticModel,
) -> Result<(Edit, String)> {
) -> Result<(Edit, String), ResolutionError> {
if let Some(stmt) = self.find_import_from(module, at) {
// Case 1: `from functools import lru_cache` is in scope, and we're trying to reference
// `functools.cache`; thus, we add `cache` to the import, and return `"cache"` as the
// bound name.
if semantic_model.is_unbound(member) {
let import_edit = self.add_member(stmt, member)?;
let Ok(import_edit) = self.add_member(stmt, member) else {
return Err(ResolutionError::InvalidEdit);
};
Ok((import_edit, member.to_string()))
} else {
bail!("Unable to insert `{member}` into scope due to name conflict")
Err(ResolutionError::ConflictingName(member.to_string()))
}
} else {
// Case 2: No `functools` import is in scope; thus, we add `import functools`, and
Expand All @@ -163,7 +161,7 @@ impl<'a> Importer<'a> {
let import_edit = self.add_import(&AnyImport::Import(Import::module(module)), at);
Ok((import_edit, format!("{module}.{member}")))
} else {
bail!("Unable to insert `{module}` into scope due to name conflict")
Err(ResolutionError::ConflictingName(module.to_string()))
}
}
}
Expand Down Expand Up @@ -224,12 +222,38 @@ impl<'a> Importer<'a> {
}
}

enum Resolution {
/// The symbol is available for use.
Success(Edit, String),
/// The result of an [`Importer::get_or_import_symbol`] call.
#[derive(Debug)]
pub(crate) enum ResolutionError {
/// The symbol is imported, but the import came after the current location.
LateBinding,
ImportAfterUsage,
/// The symbol is imported, but in an incompatible context (e.g., in typing-only context, while
/// we're in a runtime context).
IncompatibleContext,
/// The symbol can't be imported, because another symbol is bound to the same name.
ConflictingName(String),
/// The symbol can't be imported due to an error in editing an existing import statement.
InvalidEdit,
}

impl std::fmt::Display for ResolutionError {
fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ResolutionError::ImportAfterUsage => {
fmt.write_str("Unable to use existing symbol due to late binding")
}
ResolutionError::IncompatibleContext => {
fmt.write_str("Unable to use existing symbol due to incompatible context")
}
ResolutionError::ConflictingName(binding) => std::write!(
fmt,
"Unable to insert `{binding}` into scope due to name conflict"
),
ResolutionError::InvalidEdit => {
fmt.write_str("Unable to modify existing import statement")
}
}
}
}

impl Error for ResolutionError {}