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

Handle io errors gracefully #5611

Merged
merged 4 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
4 changes: 4 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ filetime = { version = "0.2.20" }
glob = { version = "0.3.1" }
globset = { version = "0.4.10" }
ignore = { version = "0.4.20" }
insta = { version = "1.31.0" }
insta = { version = "1.31.0", feature = ["filters", "glob"] }
is-macro = { version = "0.2.2" }
itertools = { version = "0.10.5" }
log = { version = "0.4.17" }
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ pretty_assertions = "1.3.0"
test-case = { workspace = true }
# Disable colored output in tests
colored = { workspace = true, features = ["no-color"] }
tempfile = "3.6.0"

[features]
default = []
Expand Down
35 changes: 20 additions & 15 deletions crates/ruff/src/pyproject_toml.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use anyhow::Result;
use colored::Colorize;
use log::warn;
use pyproject_toml::{BuildSystem, Project};
use ruff_text_size::{TextRange, TextSize};
use serde::{Deserialize, Serialize};
Expand All @@ -22,34 +23,38 @@ struct PyProjectToml {
project: Option<Project>,
}

pub fn lint_pyproject_toml(source_file: SourceFile, settings: &Settings) -> Result<Vec<Message>> {
let mut messages = vec![];

let err = match toml::from_str::<PyProjectToml>(source_file.source_text()) {
Ok(_) => return Ok(messages),
Err(err) => err,
pub fn lint_pyproject_toml(source_file: SourceFile, settings: &Settings) -> Vec<Message> {
let Some(err) = toml::from_str::<PyProjectToml>(source_file.source_text()).err() else {
return Vec::default();
};

let mut messages = Vec::new();
let range = match err.span() {
// This is bad but sometimes toml and/or serde just don't give us spans
// TODO(konstin,micha): https://github.com/astral-sh/ruff/issues/4571
None => TextRange::default(),
Some(range) => {
let Ok(end) = TextSize::try_from(range.end) else {
let message = format!(
"{} is larger than 4GB, but ruff assumes all files to be smaller",
source_file.name(),
);
if settings.rules.enabled(Rule::IOError) {
let diagnostic = Diagnostic::new(
IOError {
message: "pyproject.toml is larger than 4GB".to_string(),
},
TextRange::default(),
);
let diagnostic = Diagnostic::new(IOError { message }, TextRange::default());
messages.push(Message::from_diagnostic(
diagnostic,
source_file,
TextSize::default(),
));
} else {
warn!(
"{}{}{} {message}",
"Failed to lint ".bold(),
source_file.name().bold(),
":".bold()
);
}
return Ok(messages);
return messages;
};
TextRange::new(
// start <= end, so if end < 4GB follows start < 4GB
Expand All @@ -69,5 +74,5 @@ pub fn lint_pyproject_toml(source_file: SourceFile, settings: &Settings) -> Resu
));
}

Ok(messages)
messages
}
2 changes: 1 addition & 1 deletion crates/ruff/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ mod tests {
let messages = lint_pyproject_toml(
source_file,
&settings::Settings::for_rule(Rule::InvalidPyprojectToml),
)?;
);
assert_messages!(snapshot, messages);
Ok(())
}
Expand Down
2 changes: 0 additions & 2 deletions crates/ruff/src/settings/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,15 +293,13 @@ impl Settings {
})
}

#[cfg(test)]
pub fn for_rule(rule_code: Rule) -> Self {
Self {
rules: RuleTable::from_iter([rule_code]),
..Self::default()
}
}

#[cfg(test)]
pub fn for_rules(rules: impl IntoIterator<Item = Rule>) -> Self {
Self {
rules: RuleTable::from_iter(rules),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ wild = { version = "2" }

[dev-dependencies]
assert_cmd = { version = "2.0.8" }
insta = { workspace = true, features = ["filters"] }
tempfile = "3.6.0"
ureq = { version = "2.6.2", features = [] }

[target.'cfg(target_os = "windows")'.dependencies]
Expand Down
10 changes: 9 additions & 1 deletion crates/ruff_cli/src/bin/ruff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,15 @@ pub fn main() -> ExitCode {
Err(err) => {
#[allow(clippy::print_stderr)]
{
eprintln!("{}{} {err:?}", "error".red().bold(), ":".bold());
// This communicates that this isn't a linter error but ruff itself hard-errored for
// some reason (e.g. failed to resolve the configuration)
eprintln!("{}", "ruff failed".red().bold());
// Currently we generally only see one error, but e.g. with io errors when resolving
// the configuration it is help to chain errors ("resolving configuration failed" ->
// "failed to read file: subdir/pyproject.toml")
for cause in err.chain() {
eprintln!(" {} {cause}", "Caused by:".bold());
}
}
ExitStatus::Error.into()
}
Expand Down
100 changes: 91 additions & 9 deletions crates/ruff_cli/src/commands/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,30 +143,30 @@ pub(crate) fn run(
}
.unwrap_or_else(|(path, message)| {
if let Some(path) = &path {
error!(
"{}{}{} {message}",
"Failed to lint ".bold(),
fs::relativize_path(path).bold(),
":".bold()
);
let settings = resolver.resolve(path, pyproject_config);
if settings.rules.enabled(Rule::IOError) {
let file =
let dummy =
SourceFileBuilder::new(path.to_string_lossy().as_ref(), "").finish();

Diagnostics::new(
vec![Message::from_diagnostic(
Diagnostic::new(IOError { message }, TextRange::default()),
file,
dummy,
TextSize::default(),
)],
ImportMap::default(),
)
} else {
warn!(
"{}{}{} {message}",
"Failed to lint ".bold(),
fs::relativize_path(path).bold(),
":".bold()
);
Diagnostics::default()
}
} else {
error!("{} {message}", "Encountered error:".bold());
warn!("{} {message}", "Encountered error:".bold());
Diagnostics::default()
}
})
Expand Down Expand Up @@ -226,3 +226,85 @@ with the relevant file contents, the `pyproject.toml` settings, and the followin
}
}
}

#[cfg(test)]
#[cfg(unix)] // If you need to add a second test, move this cfg
mod test {
use super::run;
use crate::args::Overrides;
use anyhow::Result;
use ruff::message::{Emitter, EmitterContext, TextEmitter};
use ruff::registry::Rule;
use ruff::resolver::{PyprojectConfig, PyprojectDiscoveryStrategy};
use ruff::settings::{flags, AllSettings, CliSettings, Settings};
use rustc_hash::FxHashMap;
use std::fs;
use std::os::unix::fs::OpenOptionsExt;
use tempfile::TempDir;

/// We check that regular python files, pyproject.toml and jupyter notebooks all handle io
/// errors gracefully
#[test]
fn unreadable_files() -> Result<()> {
let path = "E902.py";
let rule_code = Rule::IOError;

// Create inaccessible files
let tempdir = TempDir::new()?;
let pyproject_toml = tempdir.path().join("pyproject.toml");
let python_file = tempdir.path().join("code.py");
let notebook = tempdir.path().join("notebook.ipynb");
for file in [&pyproject_toml, &python_file, &notebook] {
fs::OpenOptions::new()
.create(true)
.write(true)
.mode(0o000)
.open(file)?;
}

// Configure
let snapshot = format!("{}_{}", rule_code.noqa_code(), path);
let settings = AllSettings {
cli: CliSettings::default(),
// invalid pyproject.toml is not active by default
lib: Settings::for_rules(vec![rule_code, Rule::InvalidPyprojectToml]),
};
let pyproject_config =
PyprojectConfig::new(PyprojectDiscoveryStrategy::Fixed, settings, None);

// Run
let diagnostics = run(
// Notebooks are not included by default
&[tempdir.path().to_path_buf(), notebook],
&pyproject_config,
&Overrides::default(),
flags::Cache::Disabled,
flags::Noqa::Disabled,
flags::FixMode::Generate,
)
.unwrap();
let mut output = Vec::new();

TextEmitter::default()
.with_show_fix_status(true)
.emit(
&mut output,
&diagnostics.messages,
&EmitterContext::new(&FxHashMap::default()),
)
.unwrap();

let messages = String::from_utf8(output).unwrap();

insta::with_settings!({
omit_expression => true,
filters => vec![
// The tempdir is always different (and platform dependent)
(tempdir.path().to_str().unwrap(), "/home/ferris/project"),
]
}, {
insta::assert_snapshot!(snapshot, messages);
});
Ok(())
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
source: crates/ruff_cli/src/commands/run.rs
---
/home/ferris/project/code.py:1:1: E902 Permission denied (os error 13)
/home/ferris/project/notebook.ipynb:1:1: E902 Permission denied (os error 13)
/home/ferris/project/pyproject.toml:1:1: E902 Permission denied (os error 13)

51 changes: 45 additions & 6 deletions crates/ruff_cli/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,21 @@ use std::path::Path;

use anyhow::{anyhow, Result};
use colored::Colorize;
use log::{debug, error};
use ruff_text_size::TextSize;
use log::{debug, error, warn};
use ruff_text_size::{TextRange, TextSize};
use rustc_hash::FxHashMap;
use similar::TextDiff;

use ruff::fs;
use ruff::jupyter::Notebook;
use ruff::linter::{lint_fix, lint_only, FixTable, FixerResult, LinterResult};
use ruff::logging::DisplayParseError;
use ruff::message::Message;
use ruff::pyproject_toml::lint_pyproject_toml;
use ruff::registry::Rule;
use ruff::settings::{flags, AllSettings, Settings};
use ruff::source_kind::SourceKind;
use ruff::{fs, IOError};
use ruff_diagnostics::Diagnostic;
use ruff_python_ast::imports::ImportMap;
use ruff_python_ast::source_code::{LineIndex, SourceCode, SourceFileBuilder};
use ruff_python_stdlib::path::{is_jupyter_notebook, is_project_toml};
Expand Down Expand Up @@ -127,6 +129,31 @@ pub(crate) fn lint_path(

debug!("Checking: {}", path.display());

// In case of an io error we want to exit early
let io_error_diagnostics = |err: io::Error, path: &Path| -> Diagnostics {
if settings.lib.rules.enabled(Rule::IOError) {
let io_err = Diagnostic::new(
IOError {
message: err.to_string(),
},
TextRange::default(),
);
let dummy = SourceFileBuilder::new(path.to_string_lossy().as_ref(), "").finish();
Diagnostics::new(
vec![Message::from_diagnostic(io_err, dummy, TextSize::default())],
ImportMap::default(),
)
} else {
warn!(
"{}{}{} {err}",
"Failed to lint ".bold(),
fs::relativize_path(path).bold(),
":".bold()
);
Diagnostics::default()
}
};

// We have to special case this here since the Python tokenizer doesn't work with TOML.
if is_project_toml(path) {
let messages = if settings
Expand All @@ -135,9 +162,14 @@ pub(crate) fn lint_path(
.iter_enabled()
.any(|rule_code| rule_code.lint_source().is_pyproject_toml())
{
let contents = std::fs::read_to_string(path)?;
let contents = match std::fs::read_to_string(path) {
Ok(contents) => contents,
Err(err) => {
return Ok(io_error_diagnostics(err, path));
}
};
let source_file = SourceFileBuilder::new(path.to_string_lossy(), contents).finish();
lint_pyproject_toml(source_file, &settings.lib)?
Copy link
Member

Choose a reason for hiding this comment

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

Are these not already treated as IOError via the IOError handling in run.rs? Like this block:

error!(
    "{}{}{} {message}",
    "Failed to lint ".bold(),
    fs::relativize_path(path).bold(),
    ":".bold()
);
let settings = resolver.resolve(path, pyproject_config);
if settings.rules.enabled(Rule::IOError) {
    let file =
        SourceFileBuilder::new(path.to_string_lossy().as_ref(), "").finish();

    Diagnostics::new(
        vec![Message::from_diagnostic(
            Diagnostic::new(IOError { message }, TextRange::default()),
            file,
            TextSize::default(),
        )],
        ImportMap::default(),
    )
} else {
    Diagnostics::default()
}

Copy link
Member

Choose a reason for hiding this comment

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

With this change, do we not lose that warning message that appears separately from the diagnostics?

lint_pyproject_toml(source_file, &settings.lib)
} else {
vec![]
};
Expand All @@ -154,7 +186,14 @@ pub(crate) fn lint_path(
Err(diagnostic) => return Ok(*diagnostic),
}
} else {
SourceKind::Python(std::fs::read_to_string(path)?)
// This is tested by ruff_cli integration test `unreadable_file`
let contents = match std::fs::read_to_string(path) {
Ok(contents) => contents,
Err(err) => {
return Ok(io_error_diagnostics(err, path));
}
};
SourceKind::Python(contents)
};

let contents = source_kind.content().to_string();
Expand Down
Loading