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

Update CLI to respect fix applicability #7769

Merged
merged 29 commits into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
d01969c
Update CLI to respect fix applicability
evanrittenhouse Oct 2, 2023
74acdf5
Fixups for CI
zanieb Oct 2, 2023
85b5efe
Update CLI documentation for fixes
zanieb Oct 3, 2023
7c05fb5
Use `self.flags.intersects` consistently
zanieb Oct 3, 2023
87def58
Revert change to notebook cell iteration
zanieb Oct 3, 2023
6577833
Remove stale todo from test
zanieb Oct 3, 2023
d004ddc
Cleanup internal commentary on the fix rules
zanieb Oct 3, 2023
96b8ce1
Refactor `fix_file` implementation to use `Fix.applies`
zanieb Oct 3, 2023
9949fa0
Use different symbols for fix messages depending on applicability
zanieb Oct 3, 2023
ee903bf
Update snapshots to reflect changes in fix messages
zanieb Oct 3, 2023
b24d9dd
Remove comment detailing ordering
zanieb Oct 4, 2023
a5ef7d6
Refactor `RuleCodeAndBody.fmt` to avoid using `unwrap`
zanieb Oct 4, 2023
cb6c25e
Do not display fixes with manual applicability
zanieb Oct 4, 2023
8683340
Rename `--suggested-fix` to `--unsafe-fixes` and fix interaction with…
zanieb Oct 4, 2023
d3f0558
Improve messaging around unsafe fixes
zanieb Oct 4, 2023
d35d68d
Refactor `UnsafeFixes` out of `FixMode`
zanieb Oct 5, 2023
953ee4a
Merge branch 'main' into zanie/applicability
zanieb Oct 5, 2023
98c2c29
Update order of applicability levels for `Ord`
zanieb Oct 5, 2023
f57b853
Fix merge compile errors
zanieb Oct 5, 2023
a57e3d3
Refactor unsafe fix display to avoid using flags
zanieb Oct 5, 2023
d3cb905
Fix unsafe fixes in grouped emitter
zanieb Oct 5, 2023
2a5f454
Refactor `FixableStatistics`
zanieb Oct 5, 2023
8d89499
Lint
zanieb Oct 5, 2023
b72ab24
Fixup `CheckCommand`
zanieb Oct 5, 2023
356f409
`FixableStatistics` -> `FixableSummary`
zanieb Oct 5, 2023
1bbc858
Wrap --fix and --unsafe-fixes in code quotes
zanieb Oct 5, 2023
b6e3a9a
Clean up integration tests
zanieb Oct 5, 2023
47b146b
Commit missing snapshot
zanieb Oct 5, 2023
859ac7d
Avoid reading `unsafe_fixes` from nested configuration files
zanieb Oct 6, 2023
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
22 changes: 17 additions & 5 deletions crates/ruff_cli/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use ruff_linter::logging::LogLevel;
use ruff_linter::registry::Rule;
use ruff_linter::settings::types::{
FilePattern, PatternPrefixPair, PerFileIgnore, PreviewMode, PythonVersion, SerializationFormat,
UnsafeFixes,
};
use ruff_linter::{RuleParser, RuleSelector, RuleSelectorParser};
use ruff_workspace::configuration::{Configuration, RuleSelection};
Expand Down Expand Up @@ -76,12 +77,17 @@ pub enum Command {
pub struct CheckCommand {
/// List of files or directories to check.
pub files: Vec<PathBuf>,
/// Attempt to automatically fix lint violations.
/// Use `--no-fix` to disable.
/// Apply fixes to resolve lint violations.
/// Use `--no-fix` to disable or `--unsafe-fixes` to include suggested fixes.
#[arg(long, overrides_with("no_fix"))]
fix: bool,
#[clap(long, overrides_with("fix"), hide = true)]
#[clap(long, overrides_with_all(["fix"]), hide = true)]
no_fix: bool,
/// Include fixes that may not retain the original intent of the code.
#[arg(long, overrides_with("no_unsafe_fixes"))]
unsafe_fixes: bool,
#[arg(long, overrides_with("unsafe_fixes"))]
no_unsafe_fixes: bool,
/// Show violations with source code.
/// Use `--no-show-source` to disable.
#[arg(long, overrides_with("no_show_source"))]
Expand All @@ -100,8 +106,8 @@ pub struct CheckCommand {
/// Run in watch mode by re-running whenever files change.
#[arg(short, long)]
pub watch: bool,
/// Fix any fixable lint violations, but don't report on leftover violations. Implies `--fix`.
/// Use `--no-fix-only` to disable.
/// Apply fixes to resolve lint violations, but don't report on leftover violations. Implies `--fix`.
/// Use `--no-fix-only` to disable or `--unsafe-fixes` to include suggested fixes.
#[arg(long, overrides_with("no_fix_only"))]
fix_only: bool,
#[clap(long, overrides_with("fix_only"), hide = true)]
Expand Down Expand Up @@ -497,6 +503,8 @@ impl CheckCommand {
cache_dir: self.cache_dir,
fix: resolve_bool_arg(self.fix, self.no_fix),
fix_only: resolve_bool_arg(self.fix_only, self.no_fix_only),
unsafe_fixes: resolve_bool_arg(self.unsafe_fixes, self.no_unsafe_fixes)
.map(UnsafeFixes::from),
force_exclude: resolve_bool_arg(self.force_exclude, self.no_force_exclude),
output_format: self.output_format.or(self.format),
show_fixes: resolve_bool_arg(self.show_fixes, self.no_show_fixes),
Expand Down Expand Up @@ -599,6 +607,7 @@ pub struct CliOverrides {
pub cache_dir: Option<PathBuf>,
pub fix: Option<bool>,
pub fix_only: Option<bool>,
pub unsafe_fixes: Option<UnsafeFixes>,
pub force_exclude: Option<bool>,
pub output_format: Option<SerializationFormat>,
pub show_fixes: Option<bool>,
Expand All @@ -624,6 +633,9 @@ impl ConfigurationTransformer for CliOverrides {
if let Some(fix_only) = &self.fix_only {
config.fix_only = Some(*fix_only);
}
if self.unsafe_fixes.is_some() {
config.unsafe_fixes = self.unsafe_fixes;
}
config.lint.rule_selections.push(RuleSelection {
select: self.select.clone(),
ignore: self
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_cli/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ pub(crate) fn init(path: &Path) -> Result<()> {
#[cfg(test)]
mod tests {
use filetime::{set_file_mtime, FileTime};
use ruff_linter::settings::types::UnsafeFixes;
use std::env::temp_dir;
use std::fs;
use std::io;
Expand Down Expand Up @@ -410,6 +411,7 @@ mod tests {
Some(&cache),
flags::Noqa::Enabled,
flags::FixMode::Generate,
UnsafeFixes::Enabled,
)
.unwrap();
if diagnostics
Expand Down Expand Up @@ -455,6 +457,7 @@ mod tests {
Some(&cache),
flags::Noqa::Enabled,
flags::FixMode::Generate,
UnsafeFixes::Enabled,
)
.unwrap();
}
Expand Down Expand Up @@ -712,6 +715,7 @@ mod tests {
Some(cache),
flags::Noqa::Enabled,
flags::FixMode::Generate,
UnsafeFixes::Enabled,
)
}
}
Expand Down
16 changes: 14 additions & 2 deletions crates/ruff_cli/src/commands/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use itertools::Itertools;
use log::{debug, error, warn};
#[cfg(not(target_family = "wasm"))]
use rayon::prelude::*;
use ruff_linter::settings::types::UnsafeFixes;
use rustc_hash::FxHashMap;

use ruff_diagnostics::Diagnostic;
Expand Down Expand Up @@ -119,7 +120,16 @@ pub(crate) fn check(
}
});

lint_path(path, package, &settings.linter, cache, noqa, fix_mode).map_err(|e| {
lint_path(
path,
package,
&settings.linter,
cache,
noqa,
fix_mode,
settings.unsafe_fixes,
Copy link
Member

Choose a reason for hiding this comment

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

I think accessing from settings here could lead to inconsistencies, since the Printer and Emitter and such will use the "global" value, but accessing from settings here will read the unsafe-fixes from a nested configuration file -- if I understand correctly.

For example... if you had unsafe-fixes = true in a pyproject.toml at the top of the project, and then unsafe-fixes = false in a pyproject.toml in a subdirectory, wouldn't the CLI think you ran with unsafe-fixes, where internally you would not run with unsafe-fixes when analyzing files in the subdirectory?

I suspect this should be passed down from the top-level like fix_mode for consistency throughout the execution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm okay let me investigate that

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah you're right! Good catch — I misunderstood that call hierarchy.

)
.map_err(|e| {
(Some(path.to_owned()), {
let mut error = e.to_string();
for cause in e.chain() {
Expand Down Expand Up @@ -199,9 +209,10 @@ fn lint_path(
cache: Option<&Cache>,
noqa: flags::Noqa,
fix_mode: flags::FixMode,
unsafe_fixes: UnsafeFixes,
) -> Result<Diagnostics> {
let result = catch_unwind(|| {
crate::diagnostics::lint_path(path, package, settings, cache, noqa, fix_mode)
crate::diagnostics::lint_path(path, package, settings, cache, noqa, fix_mode, unsafe_fixes)
});

match result {
Expand Down Expand Up @@ -233,6 +244,7 @@ mod test {
use std::os::unix::fs::OpenOptionsExt;

use anyhow::Result;

use rustc_hash::FxHashMap;
use tempfile::TempDir;

Expand Down
14 changes: 12 additions & 2 deletions crates/ruff_cli/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use anyhow::{Context, Result};
use colored::Colorize;
use filetime::FileTime;
use log::{debug, error, warn};
use ruff_linter::settings::types::UnsafeFixes;
use rustc_hash::FxHashMap;

use ruff_diagnostics::Diagnostic;
Expand Down Expand Up @@ -168,6 +169,7 @@ pub(crate) fn lint_path(
cache: Option<&Cache>,
noqa: flags::Noqa,
fix_mode: flags::FixMode,
unsafe_fixes: UnsafeFixes,
) -> Result<Diagnostics> {
// Check the cache.
// TODO(charlie): `fixer::Mode::Apply` and `fixer::Mode::Diff` both have
Expand Down Expand Up @@ -244,8 +246,15 @@ pub(crate) fn lint_path(
result,
transformed,
fixed,
}) = lint_fix(path, package, noqa, settings, &source_kind, source_type)
{
}) = lint_fix(
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 reverted some changes around here to minimize the diff

path,
package,
noqa,
unsafe_fixes,
settings,
&source_kind,
source_type,
) {
Copy link
Member

Choose a reason for hiding this comment

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

I think Konsti's suggestion (which it's totally up to you whether you take) was to do, like:

let var = lint_fix(...);
if let Ok(FixerResult { ... }) = var {
  ....
}

...to avoid having a long expanded call on the RHS of the if-let. Again, completely up to you.

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'll do that separately. I want to use match instead of if matches! / else just outside this too.

if !fixed.is_empty() {
match fix_mode {
flags::FixMode::Apply => transformed.write(&mut File::create(path)?)?,
Expand Down Expand Up @@ -355,6 +364,7 @@ pub(crate) fn lint_stdin(
path.unwrap_or_else(|| Path::new("-")),
package,
noqa,
settings.unsafe_fixes,
&settings.linter,
&source_kind,
source_type,
Expand Down
28 changes: 19 additions & 9 deletions crates/ruff_cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use log::warn;
use notify::{recommended_watcher, RecursiveMode, Watcher};

use ruff_linter::logging::{set_up_logging, LogLevel};
use ruff_linter::settings::flags;
use ruff_linter::settings::flags::FixMode;
use ruff_linter::settings::types::SerializationFormat;
use ruff_linter::{fs, warn_user, warn_user_once};
use ruff_workspace::Settings;
Expand Down Expand Up @@ -228,6 +228,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
let Settings {
fix,
fix_only,
unsafe_fixes,
output_format,
show_fixes,
show_source,
Expand All @@ -236,17 +237,20 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {

// Fix rules are as follows:
// - By default, generate all fixes, but don't apply them to the filesystem.
// - If `--fix` or `--fix-only` is set, always apply fixes to the filesystem (or
// - If `--fix` or `--fix-only` is set, apply applicable fixes to the filesystem (or
// print them to stdout, if we're reading from stdin).
// - If `--diff` or `--fix-only` are set, don't print any violations (only
// fixes).
// - If `--diff` or `--fix-only` are set, don't print any violations (only applicable fixes)
// - By default, applicable fixes only include [`Applicablility::Automatic`], but if
// `--unsafe-fixes` is set, then [`Applicablility::Suggested`] fixes are included.

let fix_mode = if cli.diff {
flags::FixMode::Diff
FixMode::Diff
} else if fix || fix_only {
flags::FixMode::Apply
FixMode::Apply
} else {
flags::FixMode::Generate
FixMode::Generate
};

let cache = !cli.no_cache;
let noqa = !cli.ignore_noqa;
let mut printer_flags = PrinterFlags::empty();
Expand Down Expand Up @@ -290,7 +294,13 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
return Ok(ExitStatus::Success);
}

let printer = Printer::new(output_format, log_level, fix_mode, printer_flags);
let printer = Printer::new(
output_format,
log_level,
fix_mode,
unsafe_fixes,
printer_flags,
);

if cli.watch {
if output_format != SerializationFormat::Text {
Expand Down Expand Up @@ -382,7 +392,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
// Always try to print violations (the printer itself may suppress output),
// unless we're writing fixes via stdin (in which case, the transformed
// source code goes to stdout).
if !(is_stdin && matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff)) {
if !(is_stdin && matches!(fix_mode, FixMode::Apply | FixMode::Diff)) {
if cli.statistics {
printer.write_statistics(&diagnostics, &mut writer)?;
} else {
Expand Down
Loading
Loading