Skip to content

Commit

Permalink
Avoid failed assertion when showing fixes from stdin (#8029)
Browse files Browse the repository at this point in the history
## Summary

When linting, we store a map from file path to fixes, which we then use
to show a fix summary in the printer.

In the printer, we assume that if the map is non-empty, then we have at
least one fix. But this isn't enforced by the fix struct, since you can
have an entry from (file path) to (empty fix table). In practice, this
only bites us when linting from `stdin`, since when linting across
multiple files, we have an `AddAssign` on `Diagnostics` that avoids
adding empty entries to the map. When linting from `stdin`, we create
the map directly, and so it _is_ possible to have a non-empty map that
doesn't contain any fixes, leading to a panic.

This PR introduces a dedicated struct to make these constraints part of
the formal interface.

Closes #8027.

## Test Plan

`cargo test` (notice two failures are removed)
  • Loading branch information
charliermarsh committed Oct 18, 2023
1 parent a62c735 commit 195c000
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 16 deletions.
53 changes: 45 additions & 8 deletions crates/ruff_cli/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ 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 All @@ -20,6 +19,7 @@ use ruff_linter::logging::DisplayParseError;
use ruff_linter::message::Message;
use ruff_linter::pyproject_toml::lint_pyproject_toml;
use ruff_linter::registry::AsRule;
use ruff_linter::settings::types::UnsafeFixes;
use ruff_linter::settings::{flags, LinterSettings};
use ruff_linter::source_kind::{SourceError, SourceKind};
use ruff_linter::{fs, IOError, SyntaxError};
Expand Down Expand Up @@ -61,7 +61,7 @@ impl FileCacheKey {
#[derive(Debug, Default, PartialEq)]
pub(crate) struct Diagnostics {
pub(crate) messages: Vec<Message>,
pub(crate) fixed: FxHashMap<String, FixTable>,
pub(crate) fixed: FixMap,
pub(crate) imports: ImportMap,
pub(crate) notebook_indexes: FxHashMap<String, NotebookIndex>,
}
Expand All @@ -74,7 +74,7 @@ impl Diagnostics {
) -> Self {
Self {
messages,
fixed: FxHashMap::default(),
fixed: FixMap::default(),
imports,
notebook_indexes,
}
Expand Down Expand Up @@ -155,18 +155,55 @@ impl AddAssign for Diagnostics {
fn add_assign(&mut self, other: Self) {
self.messages.extend(other.messages);
self.imports.extend(other.imports);
for (filename, fixed) in other.fixed {
self.fixed += other.fixed;
self.notebook_indexes.extend(other.notebook_indexes);
}
}

/// A collection of fixes indexed by file path.
#[derive(Debug, Default, PartialEq)]
pub(crate) struct FixMap(FxHashMap<String, FixTable>);

impl FixMap {
/// Returns `true` if there are no fixes in the map.
pub(crate) fn is_empty(&self) -> bool {
self.0.is_empty()
}

/// Returns an iterator over the fixes in the map, along with the file path.
pub(crate) fn iter(&self) -> impl Iterator<Item = (&String, &FixTable)> {
self.0.iter()
}

/// Returns an iterator over the fixes in the map.
pub(crate) fn values(&self) -> impl Iterator<Item = &FixTable> {
self.0.values()
}
}

impl FromIterator<(String, FixTable)> for FixMap {
fn from_iter<T: IntoIterator<Item = (String, FixTable)>>(iter: T) -> Self {
Self(
iter.into_iter()
.filter(|(_, fixes)| !fixes.is_empty())
.collect(),
)
}
}

impl AddAssign for FixMap {
fn add_assign(&mut self, rhs: Self) {
for (filename, fixed) in rhs.0 {
if fixed.is_empty() {
continue;
}
let fixed_in_file = self.fixed.entry(filename).or_default();
let fixed_in_file = self.0.entry(filename).or_default();
for (rule, count) in fixed {
if count > 0 {
*fixed_in_file.entry(rule).or_default() += count;
}
}
}
self.notebook_indexes.extend(other.notebook_indexes);
}
}

Expand Down Expand Up @@ -327,7 +364,7 @@ pub(crate) fn lint_path(

Ok(Diagnostics {
messages,
fixed: FxHashMap::from_iter([(fs::relativize_path(path), fixed)]),
fixed: FixMap::from_iter([(fs::relativize_path(path), fixed)]),
imports,
notebook_indexes,
})
Expand Down Expand Up @@ -445,7 +482,7 @@ pub(crate) fn lint_stdin(

Ok(Diagnostics {
messages,
fixed: FxHashMap::from_iter([(
fixed: FixMap::from_iter([(
fs::relativize_path(path.unwrap_or_else(|| Path::new("-"))),
fixed,
)]),
Expand Down
6 changes: 2 additions & 4 deletions crates/ruff_cli/src/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@ use anyhow::Result;
use bitflags::bitflags;
use colored::Colorize;
use itertools::{iterate, Itertools};
use rustc_hash::FxHashMap;
use serde::Serialize;

use ruff_linter::fs::relativize_path;
use ruff_linter::linter::FixTable;
use ruff_linter::logging::LogLevel;
use ruff_linter::message::{
AzureEmitter, Emitter, EmitterContext, GithubEmitter, GitlabEmitter, GroupedEmitter,
Expand All @@ -22,7 +20,7 @@ use ruff_linter::registry::{AsRule, Rule};
use ruff_linter::settings::flags::{self};
use ruff_linter::settings::types::{SerializationFormat, UnsafeFixes};

use crate::diagnostics::Diagnostics;
use crate::diagnostics::{Diagnostics, FixMap};

bitflags! {
#[derive(Default, Debug, Copy, Clone)]
Expand Down Expand Up @@ -462,7 +460,7 @@ fn show_fix_status(fix_mode: flags::FixMode, fixables: Option<&FixableStatistics
(!fix_mode.is_apply()) && fixables.is_some_and(FixableStatistics::any_applicable_fixes)
}

fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap<String, FixTable>) -> Result<()> {
fn print_fix_summary(writer: &mut dyn Write, fixed: &FixMap) -> Result<()> {
let total = fixed
.values()
.map(|table| table.values().sum::<usize>())
Expand Down
8 changes: 4 additions & 4 deletions crates/ruff_cli/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1252,8 +1252,8 @@ fn diff_does_not_show_display_only_fixes_with_unsafe_fixes_enabled() {
])
.pass_stdin("def add_to_list(item, some_list=[]): ..."),
@r###"
success: false
exit_code: 1
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Expand All @@ -1276,8 +1276,8 @@ fn diff_only_unsafe_fixes_available() {
])
.pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"),
@r###"
success: false
exit_code: 1
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Expand Down

0 comments on commit 195c000

Please sign in to comment.