Skip to content

Commit

Permalink
Handle io errors gracefully
Browse files Browse the repository at this point in the history
  • Loading branch information
konstin committed Jul 11, 2023
1 parent 0c8ec80 commit b87738a
Show file tree
Hide file tree
Showing 11 changed files with 190 additions and 27 deletions.
3 changes: 3 additions & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions crates/ruff/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,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
30 changes: 14 additions & 16 deletions crates/ruff/src/pyproject_toml.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use anyhow::Result;
use pyproject_toml::{BuildSystem, Project};
use ruff_text_size::{TextRange, TextSize};
use serde::{Deserialize, Serialize};
Expand All @@ -22,34 +21,33 @@ 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![];
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 {
if settings.rules.enabled(Rule::IOError) {
let diagnostic = Diagnostic::new(
IOError {
message: "pyproject.toml is larger than 4GB".to_string(),
},
TextRange::default(),
return if settings.rules.enabled(Rule::IOError) {
let message = format!(
"{} is larger than 4GB, but ruff assumes all files to be smaller",
source_file.name(),
);
let diagnostic = Diagnostic::new(IOError { message }, TextRange::default());
messages.push(Message::from_diagnostic(
diagnostic,
source_file,
TextSize::default(),
));
}
return Ok(messages);
messages
} else {
Vec::new()
};
};
TextRange::new(
// start <= end, so if end < 4GB follows start < 4GB
Expand All @@ -69,5 +67,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 @@ -210,7 +210,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 }
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
83 changes: 81 additions & 2 deletions crates/ruff_cli/src/commands/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,13 @@ pub(crate) fn run(
);
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(),
Expand Down Expand Up @@ -226,3 +226,82 @@ with the relevant file contents, the `pyproject.toml` settings, and the followin
}
}
}

#[cfg(test)]
#[cfg(unix)]
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();

// Remove the tempdir from the messages
let messages = String::from_utf8(output)
.unwrap()
.replace(tempdir.path().to_str().unwrap(), "");

insta::with_settings!({ omit_expression => true }, {
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
---
/code.py:1:1: E902 Permission denied (os error 13)
/notebook.ipynb:1:1: E902 Permission denied (os error 13)
/pyproject.toml:1:1: E902 Permission denied (os error 13)

38 changes: 33 additions & 5 deletions crates/ruff_cli/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,19 @@ use std::path::Path;
use anyhow::{anyhow, Result};
use colored::Colorize;
use log::{debug, error};
use ruff_text_size::TextSize;
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::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 +128,21 @@ 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 {
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(),
)
};

// 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 +151,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)?
lint_pyproject_toml(source_file, &settings.lib)
} else {
vec![]
};
Expand All @@ -154,7 +175,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
39 changes: 39 additions & 0 deletions crates/ruff_cli/tests/integration_test.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,26 @@
#![cfg(not(target_family = "wasm"))]

#[cfg(unix)]
use std::fs;
#[cfg(unix)]
use std::os::unix::fs::OpenOptionsExt;
#[cfg(unix)]
use std::path::Path;
use std::str;

#[cfg(unix)]
use anyhow::Context;
use anyhow::Result;
use assert_cmd::Command;
#[cfg(unix)]
use clap::Parser;
#[cfg(unix)]
use path_absolutize::path_dedot;
#[cfg(unix)]
use tempfile::TempDir;

use ruff_cli::args::Args;
use ruff_cli::run;

const BIN_NAME: &str = "ruff";

Expand Down Expand Up @@ -278,3 +291,29 @@ Found 1 error.

Ok(())
}

/// An unreadable pyproject.toml in non-isolated mode causes ruff to hard-error trying to build up
/// configuration globs
#[cfg(unix)]
#[test]
fn unreadable_pyproject_toml() -> Result<()> {
let tempdir = TempDir::new()?;
let pyproject_toml = tempdir.path().join("pyproject.toml");
// Create an empty file with 000 permissions
fs::OpenOptions::new()
.create(true)
.write(true)
.mode(0o000)
.open(pyproject_toml)?;

// Don't `--isolated` since the configuration discovery is where the error happens
let args = Args::parse_from(["", "check", "--no-cache", tempdir.path().to_str().unwrap()]);
let err = run(args).err().context("Unexpected success")?;
assert_eq!(
err.chain()
.map(std::string::ToString::to_string)
.collect::<Vec<_>>(),
vec!["Permission denied (os error 13)".to_string()],
);
Ok(())
}

0 comments on commit b87738a

Please sign in to comment.