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 all 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
21 changes: 17 additions & 4 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,18 @@ 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 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.

Should we say "potentially unsafe fixes"? (Defer to you.)

Copy link
Member Author

@zanieb zanieb Oct 5, 2023

Choose a reason for hiding this comment

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

Hm.. you were the one that told me unsafe is an all or nothing concept haha I'll ponder this as well.

Should it be "potentially incorrect fixes"? Fwiw I feel like --unsafe-fixes is the place for a more detailed description.

I also considered "to include additional fixes"

Copy link
Member

Choose a reason for hiding this comment

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

Haha. I guess the way I thought about it was that a given fix is either safe or not. The collection of fixes may thereby contain unsafe changes.

#[arg(long, overrides_with("no_fix"))]
fix: bool,
#[clap(long, overrides_with("fix"), hide = true)]
no_fix: bool,
/// Include fixes that may not retain the original intent of the code.
/// Use `--no-unsafe-fixes` to disable.
#[arg(long, overrides_with("no_unsafe_fixes"))]
unsafe_fixes: bool,
#[arg(long, overrides_with("unsafe_fixes"), hide = true)]
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 +107,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 unsafe 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 +504,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 +608,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 +634,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
19 changes: 17 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 All @@ -36,6 +37,7 @@ pub(crate) fn check(
cache: flags::Cache,
noqa: flags::Noqa,
fix_mode: flags::FixMode,
unsafe_fixes: UnsafeFixes,
) -> Result<Diagnostics> {
// Collect all the Python files to check.
let start = Instant::now();
Expand Down Expand Up @@ -119,7 +121,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,
unsafe_fixes,
)
.map_err(|e| {
(Some(path.to_owned()), {
let mut error = e.to_string();
for cause in e.chain() {
Expand Down Expand Up @@ -199,9 +210,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 +245,8 @@ mod test {
use std::os::unix::fs::OpenOptionsExt;

use anyhow::Result;

use ruff_linter::settings::types::UnsafeFixes;
use rustc_hash::FxHashMap;
use tempfile::TempDir;

Expand Down Expand Up @@ -285,6 +299,7 @@ mod test {
flags::Cache::Disabled,
flags::Noqa::Disabled,
flags::FixMode::Generate,
UnsafeFixes::Enabled,
)
.unwrap();
let mut output = Vec::new();
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
31 changes: 22 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 @@ -318,6 +328,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
cache.into(),
noqa.into(),
fix_mode,
unsafe_fixes,
)?;
printer.write_continuously(&mut writer, &messages)?;

Expand Down Expand Up @@ -350,6 +361,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
cache.into(),
noqa.into(),
fix_mode,
unsafe_fixes,
)?;
printer.write_continuously(&mut writer, &messages)?;
}
Expand All @@ -376,13 +388,14 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
cache.into(),
noqa.into(),
fix_mode,
unsafe_fixes,
)?
};

// 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