Avoid failed assertion when showing fixes from stdin (#8029)

## 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 https://github.com/astral-sh/ruff/issues/8027.

## Test Plan

`cargo test` (notice two failures are removed)
This commit is contained in:
Charlie Marsh 2023-10-17 21:50:39 -04:00 committed by GitHub
parent a62c735f9e
commit 195c000f5a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 51 additions and 16 deletions

View file

@ -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;
@ -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};
@ -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>,
}
@ -74,7 +74,7 @@ impl Diagnostics {
) -> Self {
Self {
messages,
fixed: FxHashMap::default(),
fixed: FixMap::default(),
imports,
notebook_indexes,
}
@ -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);
}
}
@ -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,
})
@ -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,
)]),

View file

@ -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,
@ -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)]
@ -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>())