Skip to content

Commit

Permalink
Truncate some messages in diagnostics (#6748)
Browse files Browse the repository at this point in the history
## Summary

I noticed this in the ecosystem CI check from
#6742. If we include source code
directly in a diagnostic, we need to be careful to avoid rendering
multi-line diagnostics or even excessively long diagnostics.

## Test Plan

`cargo test`
  • Loading branch information
charliermarsh authored Aug 22, 2023
1 parent 0aad0c4 commit fa32cd9
Show file tree
Hide file tree
Showing 12 changed files with 118 additions and 57 deletions.
1 change: 1 addition & 0 deletions crates/ruff/src/autofix/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::registry::{AsRule, Rule};

pub(crate) mod codemods;
pub(crate) mod edits;
pub(crate) mod snippet;
pub(crate) mod source_map;

pub(crate) struct FixResult {
Expand Down
36 changes: 36 additions & 0 deletions crates/ruff/src/autofix/snippet.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
use unicode_width::UnicodeWidthStr;

/// A snippet of source code for user-facing display, as in a diagnostic.
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct SourceCodeSnippet(String);

impl SourceCodeSnippet {
pub(crate) fn new(source_code: String) -> Self {
Self(source_code)
}

/// Return the full snippet for user-facing display, or `None` if the snippet should be
/// truncated.
pub(crate) fn full_display(&self) -> Option<&str> {
if Self::should_truncate(&self.0) {
None
} else {
Some(&self.0)
}
}

/// Return a truncated snippet for user-facing display.
pub(crate) fn truncated_display(&self) -> &str {
if Self::should_truncate(&self.0) {
"..."
} else {
&self.0
}
}

/// Returns `true` if the source code should be truncated when included in a user-facing
/// diagnostic.
fn should_truncate(source_code: &str) -> bool {
source_code.width() > 50 || source_code.contains(['\r', '\n'])
}
}
24 changes: 16 additions & 8 deletions crates/ruff/src/rules/flake8_simplify/rules/yoda_conditions.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
use anyhow::Result;
use libcst_native::CompOp;
use ruff_python_ast::{self as ast, CmpOp, Expr, Ranged, UnaryOp};

use crate::autofix::codemods::CodegenStylist;
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, CmpOp, Expr, Ranged, UnaryOp};
use ruff_python_codegen::Stylist;
use ruff_python_stdlib::str::{self};
use ruff_source_file::Locator;

use crate::autofix::codemods::CodegenStylist;
use crate::autofix::snippet::SourceCodeSnippet;
use crate::checkers::ast::Checker;
use crate::cst::matchers::{match_comparison, match_expression};
use crate::registry::AsRule;
Expand Down Expand Up @@ -45,7 +46,7 @@ use crate::registry::AsRule;
/// - [Python documentation: Assignment statements](https://docs.python.org/3/reference/simple_stmts.html#assignment-statements)
#[violation]
pub struct YodaConditions {
pub suggestion: Option<String>,
suggestion: Option<SourceCodeSnippet>,
}

impl Violation for YodaConditions {
Expand All @@ -54,7 +55,10 @@ impl Violation for YodaConditions {
#[derive_message_formats]
fn message(&self) -> String {
let YodaConditions { suggestion } = self;
if let Some(suggestion) = suggestion {
if let Some(suggestion) = suggestion
.as_ref()
.and_then(SourceCodeSnippet::full_display)
{
format!("Yoda conditions are discouraged, use `{suggestion}` instead")
} else {
format!("Yoda conditions are discouraged")
Expand All @@ -63,9 +67,13 @@ impl Violation for YodaConditions {

fn autofix_title(&self) -> Option<String> {
let YodaConditions { suggestion } = self;
suggestion
.as_ref()
.map(|suggestion| format!("Replace Yoda condition with `{suggestion}`"))
suggestion.as_ref().map(|suggestion| {
if let Some(suggestion) = suggestion.full_display() {
format!("Replace Yoda condition with `{suggestion}`")
} else {
format!("Replace Yoda condition")
}
})
}
}

Expand Down Expand Up @@ -178,7 +186,7 @@ pub(crate) fn yoda_conditions(
if let Ok(suggestion) = reverse_comparison(expr, checker.locator(), checker.stylist()) {
let mut diagnostic = Diagnostic::new(
YodaConditions {
suggestion: Some(suggestion.to_string()),
suggestion: Some(SourceCodeSnippet::new(suggestion.clone())),
},
expr.range(),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ SIM300.py:15:1: SIM300 [*] Yoda conditions are discouraged, use `(number - 100)
17 17 |
18 18 | # OK

SIM300.py:16:1: SIM300 [*] Yoda conditions are discouraged, use `(60 * 60) < SomeClass().settings.SOME_CONSTANT_VALUE` instead
SIM300.py:16:1: SIM300 [*] Yoda conditions are discouraged
|
14 | JediOrder.YODA == age # SIM300
15 | 0 < (number - 100) # SIM300
Expand All @@ -299,7 +299,7 @@ SIM300.py:16:1: SIM300 [*] Yoda conditions are discouraged, use `(60 * 60) < Som
17 |
18 | # OK
|
= help: Replace Yoda condition with `(60 * 60) < SomeClass().settings.SOME_CONSTANT_VALUE`
= help: Replace Yoda condition

Fix
13 13 | YODA >= age # SIM300
Expand Down
25 changes: 17 additions & 8 deletions crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use itertools::Itertools;
use ruff_python_ast::{self as ast, Arguments, Constant, Expr, Ranged};
use ruff_text_size::TextRange;

use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Arguments, Constant, Expr, Ranged};
use ruff_text_size::TextRange;

use crate::autofix::snippet::SourceCodeSnippet;
use crate::checkers::ast::Checker;
use crate::registry::AsRule;
use crate::rules::flynt::helpers;
Expand All @@ -29,19 +30,27 @@ use crate::rules::flynt::helpers;
/// - [Python documentation: f-strings](https://docs.python.org/3/reference/lexical_analysis.html#f-strings)
#[violation]
pub struct StaticJoinToFString {
expr: String,
expression: SourceCodeSnippet,
}

impl AlwaysAutofixableViolation for StaticJoinToFString {
#[derive_message_formats]
fn message(&self) -> String {
let StaticJoinToFString { expr } = self;
format!("Consider `{expr}` instead of string join")
let StaticJoinToFString { expression } = self;
if let Some(expression) = expression.full_display() {
format!("Consider `{expression}` instead of string join")
} else {
format!("Consider f-string instead of string join")
}
}

fn autofix_title(&self) -> String {
let StaticJoinToFString { expr } = self;
format!("Replace with `{expr}`")
let StaticJoinToFString { expression } = self;
if let Some(expression) = expression.full_display() {
format!("Replace with `{expression}`")
} else {
format!("Replace with f-string")
}
}
}

Expand Down Expand Up @@ -140,7 +149,7 @@ pub(crate) fn static_join_to_fstring(checker: &mut Checker, expr: &Expr, joiner:

let mut diagnostic = Diagnostic::new(
StaticJoinToFString {
expr: contents.clone(),
expression: SourceCodeSnippet::new(contents.clone()),
},
expr.range(),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ pub struct BadStringFormatCharacter {
impl Violation for BadStringFormatCharacter {
#[derive_message_formats]
fn message(&self) -> String {
format!("Unsupported format character '{}'", self.format_char)
let BadStringFormatCharacter { format_char } = self;
format!("Unsupported format character '{format_char}'")
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use itertools::{any, Itertools};
use ruff_python_ast::{BoolOp, CmpOp, Expr, ExprBoolOp, ExprCompare, Ranged};
use rustc_hash::FxHashMap;

use crate::autofix::snippet::SourceCodeSnippet;
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::hashable::HashableExpr;
Expand Down Expand Up @@ -42,16 +43,22 @@ use crate::checkers::ast::Checker;
/// - [Python documentation: `set`](https://docs.python.org/3/library/stdtypes.html#set)
#[violation]
pub struct RepeatedEqualityComparisonTarget {
expr: String,
expression: SourceCodeSnippet,
}

impl Violation for RepeatedEqualityComparisonTarget {
#[derive_message_formats]
fn message(&self) -> String {
let RepeatedEqualityComparisonTarget { expr } = self;
format!(
"Consider merging multiple comparisons: `{expr}`. Use a `set` if the elements are hashable."
)
let RepeatedEqualityComparisonTarget { expression } = self;
if let Some(expression) = expression.full_display() {
format!(
"Consider merging multiple comparisons: `{expression}`. Use a `set` if the elements are hashable."
)
} else {
format!(
"Consider merging multiple comparisons. Use a `set` if the elements are hashable."
)
}
}
}

Expand Down Expand Up @@ -84,12 +91,12 @@ pub(crate) fn repeated_equality_comparison_target(checker: &mut Checker, bool_op
if count > 1 {
checker.diagnostics.push(Diagnostic::new(
RepeatedEqualityComparisonTarget {
expr: merged_membership_test(
expression: SourceCodeSnippet::new(merged_membership_test(
left.as_expr(),
bool_op.op,
&comparators,
checker.locator(),
),
)),
},
bool_op.range(),
));
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use ruff_python_ast::{self as ast, Expr, ExprContext, Operator, Ranged};
use ruff_text_size::TextRange;

use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, ExprContext, Operator, Ranged};
use ruff_text_size::TextRange;

use crate::autofix::snippet::SourceCodeSnippet;
use crate::checkers::ast::Checker;
use crate::registry::AsRule;

Expand Down Expand Up @@ -39,21 +39,29 @@ use crate::registry::AsRule;
/// - [Python documentation: Sequence Types — `list`, `tuple`, `range`](https://docs.python.org/3/library/stdtypes.html#sequence-types-list-tuple-range)
#[violation]
pub struct CollectionLiteralConcatenation {
expr: String,
expression: SourceCodeSnippet,
}

impl Violation for CollectionLiteralConcatenation {
const AUTOFIX: AutofixKind = AutofixKind::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let CollectionLiteralConcatenation { expr } = self;
format!("Consider `{expr}` instead of concatenation")
let CollectionLiteralConcatenation { expression } = self;
if let Some(expression) = expression.full_display() {
format!("Consider `{expression}` instead of concatenation")
} else {
format!("Consider iterable unpacking instead of concatenation")
}
}

fn autofix_title(&self) -> Option<String> {
let CollectionLiteralConcatenation { expr } = self;
Some(format!("Replace with `{expr}`"))
let CollectionLiteralConcatenation { expression } = self;
if let Some(expression) = expression.full_display() {
Some(format!("Replace with `{expression}`"))
} else {
Some(format!("Replace with iterable unpacking"))
}
}
}

Expand Down Expand Up @@ -186,7 +194,7 @@ pub(crate) fn collection_literal_concatenation(checker: &mut Checker, expr: &Exp
};
let mut diagnostic = Diagnostic::new(
CollectionLiteralConcatenation {
expr: contents.clone(),
expression: SourceCodeSnippet::new(contents.clone()),
},
expr.range(),
);
Expand Down
3 changes: 2 additions & 1 deletion crates/ruff/src/rules/ruff/rules/implicit_optional.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ impl Violation for ImplicitOptional {
}

fn autofix_title(&self) -> Option<String> {
Some(format!("Convert to `{}`", self.conversion_type))
let ImplicitOptional { conversion_type } = self;
Some(format!("Convert to `{conversion_type}`"))
}
}

Expand Down
3 changes: 2 additions & 1 deletion crates/ruff/src/rules/ruff/rules/invalid_pyproject_toml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ impl Violation for InvalidPyprojectToml {

#[derive_message_formats]
fn message(&self) -> String {
format!("Failed to parse pyproject.toml: {}", self.message)
let InvalidPyprojectToml { message } = self;
format!("Failed to parse pyproject.toml: {message}")
}
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
use std::borrow::Cow;

use num_traits::Zero;
use unicode_width::UnicodeWidthStr;

use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Arguments, Comprehension, Constant, Expr, Ranged};
use ruff_python_semantic::SemanticModel;
use ruff_text_size::{TextRange, TextSize};

use crate::autofix::snippet::SourceCodeSnippet;
use crate::checkers::ast::Checker;
use crate::registry::AsRule;

Expand Down Expand Up @@ -47,35 +47,24 @@ use crate::registry::AsRule;
/// - [Iterators and Iterables in Python: Run Efficient Iterations](https://realpython.com/python-iterators-iterables/#when-to-use-an-iterator-in-python)
#[violation]
pub(crate) struct UnnecessaryIterableAllocationForFirstElement {
iterable: String,
iterable: SourceCodeSnippet,
}

impl AlwaysAutofixableViolation for UnnecessaryIterableAllocationForFirstElement {
#[derive_message_formats]
fn message(&self) -> String {
let UnnecessaryIterableAllocationForFirstElement { iterable } = self;
let iterable = Self::truncate(iterable);
let iterable = iterable.truncated_display();
format!("Prefer `next({iterable})` over single element slice")
}

fn autofix_title(&self) -> String {
let UnnecessaryIterableAllocationForFirstElement { iterable } = self;
let iterable = Self::truncate(iterable);
let iterable = iterable.truncated_display();
format!("Replace with `next({iterable})`")
}
}

impl UnnecessaryIterableAllocationForFirstElement {
/// If the iterable is too long, or spans multiple lines, truncate it.
fn truncate(iterable: &str) -> &str {
if iterable.width() > 40 || iterable.contains(['\r', '\n']) {
"..."
} else {
iterable
}
}
}

/// RUF015
pub(crate) fn unnecessary_iterable_allocation_for_first_element(
checker: &mut Checker,
Expand Down Expand Up @@ -104,7 +93,7 @@ pub(crate) fn unnecessary_iterable_allocation_for_first_element(

let mut diagnostic = Diagnostic::new(
UnnecessaryIterableAllocationForFirstElement {
iterable: iterable.to_string(),
iterable: SourceCodeSnippet::new(iterable.to_string()),
},
*range,
);
Expand Down
Loading

0 comments on commit fa32cd9

Please sign in to comment.