Skip to content

Commit

Permalink
feat: custom explanations with --suppress (#4343)
Browse files Browse the repository at this point in the history
Co-authored-by: Emanuele Stoppa <[email protected]>
  • Loading branch information
anthonyshew and ematipico authored Nov 4, 2024
1 parent d650120 commit 3f152b3
Show file tree
Hide file tree
Showing 44 changed files with 338 additions and 78 deletions.
2 changes: 2 additions & 0 deletions crates/biome_analyze/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,8 @@ pub struct SuppressionCommentEmitterPayload<'a, L: Language> {
pub suppression_text: &'a str,
/// The original range of the diagnostic where the rule was triggered
pub diagnostic_text_range: &'a TextRange,
/// Explanation for the suppression to be used with `--suppress` and `--reason`
pub suppression_reason: &'a str,
}

type SignalHandler<'a, L, Break> = &'a mut dyn FnMut(&dyn AnalyzerSignal<L>) -> ControlFlow<Break>;
Expand Down
1 change: 1 addition & 0 deletions crates/biome_analyze/src/matcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ mod tests {
_: &mut BatchMutation<Self::Language>,
_: ApplySuppression<Self::Language>,
_: &str,
_: &str,
) {
unreachable!("")
}
Expand Down
3 changes: 3 additions & 0 deletions crates/biome_analyze/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ pub struct AnalyzerOptions {

/// The file that is being analyzed
pub file_path: PathBuf,

/// Suppression reason used when applying a suppression code action
pub suppression_reason: Option<String>,
}

impl AnalyzerOptions {
Expand Down
2 changes: 2 additions & 0 deletions crates/biome_analyze/src/rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,7 @@ pub trait Rule: RuleMeta + Sized {
ctx: &RuleContext<Self>,
text_range: &TextRange,
suppression_action: &dyn SuppressionAction<Language = RuleLanguage<Self>>,
suppression_reason: Option<&str>,
) -> Option<SuppressAction<RuleLanguage<Self>>>
where
Self: 'static,
Expand All @@ -901,6 +902,7 @@ pub trait Rule: RuleMeta + Sized {
mutation: &mut mutation,
token_offset: token,
diagnostic_text_range: text_range,
suppression_reason: suppression_reason.unwrap_or("<explanation>"),
});

Some(SuppressAction {
Expand Down
10 changes: 6 additions & 4 deletions crates/biome_analyze/src/signals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,6 @@ where
suppression_action: &'phase dyn SuppressionAction<
Language = <<R as Rule>::Query as Queryable>::Language,
>,

options: &'phase AnalyzerOptions,
) -> Self {
Self {
Expand Down Expand Up @@ -402,9 +401,12 @@ where
});
};
if let Some(text_range) = R::text_range(&ctx, &self.state) {
if let Some(suppression_action) =
R::suppress(&ctx, &text_range, self.suppression_action)
{
if let Some(suppression_action) = R::suppress(
&ctx,
&text_range,
self.suppression_action,
self.options.suppression_reason.as_deref(),
) {
let action = AnalyzerAction {
rule_name: Some((<R::Group as RuleGroup>::NAME, R::METADATA.name)),
category: ActionCategory::Other(Cow::Borrowed(SUPPRESSION_ACTION_CATEGORY)),
Expand Down
9 changes: 8 additions & 1 deletion crates/biome_analyze/src/suppression_action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub trait SuppressionAction {
mutation,
suppression_text,
diagnostic_text_range,
suppression_reason,
} = payload;

// retrieve the most suited, most left token where the diagnostics was emitted
Expand All @@ -22,7 +23,12 @@ pub trait SuppressionAction {
});

if let Some(apply_suppression) = apply_suppression {
self.apply_suppression(mutation, apply_suppression, suppression_text);
self.apply_suppression(
mutation,
apply_suppression,
suppression_text,
suppression_reason,
);
}
}

Expand Down Expand Up @@ -72,6 +78,7 @@ pub trait SuppressionAction {
mutation: &mut BatchMutation<Self::Language>,
apply_suppression: ApplySuppression<Self::Language>,
suppression_text: &str,
suppression_reason: &str,
);
}

Expand Down
1 change: 1 addition & 0 deletions crates/biome_analyze/src/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ mod tests {
_: &mut BatchMutation<Self::Language>,
_: ApplySuppression<Self::Language>,
_: &str,
_: &str,
) {
unreachable!("")
}
Expand Down
1 change: 1 addition & 0 deletions crates/biome_cli/src/commands/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ impl CommandRunner for CheckCommandPayload {
apply_unsafe: self.apply_unsafe,
write: self.write,
suppress: false,
suppression_reason: None,
fix: self.fix,
unsafe_: self.unsafe_,
},
Expand Down
3 changes: 3 additions & 0 deletions crates/biome_cli/src/commands/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub(crate) struct LintCommandPayload {
pub(crate) fix: bool,
pub(crate) unsafe_: bool,
pub(crate) suppress: bool,
pub(crate) suppression_reason: Option<String>,
pub(crate) linter_configuration: Option<PartialLinterConfiguration>,
pub(crate) vcs_configuration: Option<PartialVcsConfiguration>,
pub(crate) files_configuration: Option<PartialFilesConfiguration>,
Expand Down Expand Up @@ -138,6 +139,7 @@ impl CommandRunner for LintCommandPayload {
fix: self.fix,
unsafe_: self.unsafe_,
suppress: self.suppress,
suppression_reason: self.suppression_reason.clone(),
},
console,
)?;
Expand All @@ -148,6 +150,7 @@ impl CommandRunner for LintCommandPayload {
skip: self.skip.clone(),
vcs_targeted: (self.staged, self.changed).into(),
suppress: self.suppress,
suppression_reason: self.suppression_reason.clone(),
})
.set_report(cli_options))
}
Expand Down
1 change: 1 addition & 0 deletions crates/biome_cli/src/commands/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ impl CommandRunner for MigrateCommandPayload {
fix: self.fix,
unsafe_: false,
suppress: false,
suppression_reason: None,
})
}

Expand Down
62 changes: 39 additions & 23 deletions crates/biome_cli/src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,6 @@ pub enum BiomeCommand {
#[bpaf(long("write"), switch)]
write: bool,

/// Fix diagnostics with suppression comments if the language supports it.
#[bpaf(long("suppress"))]
suppress: bool,

/// Allow to do unsafe fixes, should be used with `--write` or `--fix`
#[bpaf(long("unsafe"), switch)]
unsafe_: bool,
Expand All @@ -207,6 +203,14 @@ pub enum BiomeCommand {
#[bpaf(long("apply-unsafe"), switch, hide_usage)]
apply_unsafe: bool,

/// Fixes lint rule violations with a comment a suppression instead of using a rule code action (fix)
#[bpaf(long("suppress"))]
suppress: bool,

/// Explanation for suppressing diagnostics with `--suppress`
#[bpaf(long("reason"), argument("STRING"))]
suppression_reason: Option<String>,

#[bpaf(external(partial_linter_configuration), hide_usage, optional)]
linter_configuration: Option<PartialLinterConfiguration>,

Expand Down Expand Up @@ -708,6 +712,7 @@ pub(crate) struct FixFileModeOptions {
apply_unsafe: bool,
write: bool,
suppress: bool,
suppression_reason: Option<String>,
fix: bool,
unsafe_: bool,
}
Expand All @@ -725,6 +730,7 @@ pub(crate) fn determine_fix_file_mode(
write,
fix,
suppress,
suppression_reason: _,
unsafe_,
} = options;

Expand Down Expand Up @@ -763,6 +769,7 @@ fn check_fix_incompatible_arguments(options: FixFileModeOptions) -> Result<(), C
apply_unsafe,
write,
suppress,
suppression_reason,
fix,
unsafe_,
} = options;
Expand Down Expand Up @@ -795,7 +802,12 @@ fn check_fix_incompatible_arguments(options: FixFileModeOptions) -> Result<(), C
));
} else if suppress && fix {
return Err(CliDiagnostic::incompatible_arguments("--suppress", "--fix"));
}
} else if !suppress && suppression_reason.is_some() {
return Err(CliDiagnostic::unexpected_argument(
"--reason",
"`--reason` is only valid when `--suppress` is used.",
));
};
Ok(())
}

Expand Down Expand Up @@ -983,20 +995,21 @@ mod tests {

#[test]
fn incompatible_arguments() {
for (apply, apply_unsafe, write, suppress, fix, unsafe_) in [
(true, true, false, false, false, false), // --apply --apply-unsafe
(true, false, true, false, false, false), // --apply --write
(true, false, false, false, true, false), // --apply --fix
(false, true, false, false, false, true), // --apply-unsafe --unsafe
(false, true, true, false, false, false), // --apply-unsafe --write
(false, true, false, false, true, false), // --apply-unsafe --fix
(false, false, true, false, true, false), // --write --fix
for (apply, apply_unsafe, write, suppress, suppression_reason, fix, unsafe_) in [
(true, true, false, false, None, false, false), // --apply --apply-unsafe
(true, false, true, false, None, false, false), // --apply --write
(true, false, false, false, None, true, false), // --apply --fix
(false, true, false, false, None, false, true), // --apply-unsafe --unsafe
(false, true, true, false, None, false, false), // --apply-unsafe --write
(false, true, false, false, None, true, false), // --apply-unsafe --fix
(false, false, true, false, None, true, false), // --write --fix
] {
assert!(check_fix_incompatible_arguments(FixFileModeOptions {
apply,
apply_unsafe,
write,
suppress,
suppression_reason,
fix,
unsafe_
})
Expand All @@ -1008,10 +1021,10 @@ mod tests {
fn safe_fixes() {
let mut console = BufferConsole::default();

for (apply, apply_unsafe, write, suppress, fix, unsafe_) in [
(true, false, false, false, false, false), // --apply
(false, false, true, false, false, false), // --write
(false, false, false, false, true, false), // --fix
for (apply, apply_unsafe, write, suppress, suppression_reason, fix, unsafe_) in [
(true, false, false, false, None, false, false), // --apply
(false, false, true, false, None, false, false), // --write
(false, false, false, false, None, true, false), // --fix
] {
assert_eq!(
determine_fix_file_mode(
Expand All @@ -1020,6 +1033,7 @@ mod tests {
apply_unsafe,
write,
suppress,
suppression_reason,
fix,
unsafe_
},
Expand All @@ -1035,10 +1049,10 @@ mod tests {
fn safe_and_unsafe_fixes() {
let mut console = BufferConsole::default();

for (apply, apply_unsafe, write, suppress, fix, unsafe_) in [
(false, true, false, false, false, false), // --apply-unsafe
(false, false, true, false, false, true), // --write --unsafe
(false, false, false, false, true, true), // --fix --unsafe
for (apply, apply_unsafe, write, suppress, suppression_reason, fix, unsafe_) in [
(false, true, false, false, None, false, false), // --apply-unsafe
(false, false, true, false, None, false, true), // --write --unsafe
(false, false, false, false, None, true, true), // --fix --unsafe
] {
assert_eq!(
determine_fix_file_mode(
Expand All @@ -1047,6 +1061,7 @@ mod tests {
apply_unsafe,
write,
suppress,
suppression_reason,
fix,
unsafe_
},
Expand All @@ -1062,15 +1077,16 @@ mod tests {
fn no_fix() {
let mut console = BufferConsole::default();

let (apply, apply_unsafe, write, suppress, fix, unsafe_) =
(false, false, false, false, false, false);
let (apply, apply_unsafe, write, suppress, suppression_reason, fix, unsafe_) =
(false, false, false, false, None, false, false);
assert_eq!(
determine_fix_file_mode(
FixFileModeOptions {
apply,
apply_unsafe,
write,
suppress,
suppression_reason,
fix,
unsafe_
},
Expand Down
2 changes: 2 additions & 0 deletions crates/biome_cli/src/execute/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ pub enum TraversalMode {
vcs_targeted: VcsTargeted,
/// Supress existing diagnostics with a `// biome-ignore` comment
suppress: bool,
/// Explanation for suppressing diagnostics with `--suppress` and `--reason`
suppression_reason: Option<String>,
},
/// This mode is enabled when running the command `biome ci`
CI {
Expand Down
13 changes: 11 additions & 2 deletions crates/biome_cli/src/execute/process_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,18 @@ pub(crate) fn process_file(ctx: &TraversalOptions, biome_path: &BiomePath) -> Fi
let shared_context = &SharedTraversalOptions::new(ctx);

match ctx.execution.traversal_mode {
TraversalMode::Lint { .. } => {
TraversalMode::Lint {
ref suppression_reason,
suppress,
..
} => {
// the unsupported case should be handled already at this point
lint(shared_context, biome_path)
lint(
shared_context,
biome_path,
suppress,
suppression_reason.as_deref(),
)
}
TraversalMode::Format { .. } => {
// the unsupported case should be handled already at this point
Expand Down
1 change: 1 addition & 0 deletions crates/biome_cli/src/execute/process_file/assists.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ pub(crate) fn assists_with_guard<'ctx>(
RuleCategoriesBuilder::default().with_action().build(),
only.clone(),
skip.clone(),
None,
)
.with_file_path_and_code(
workspace_file.path.display().to_string(),
Expand Down
2 changes: 1 addition & 1 deletion crates/biome_cli/src/execute/process_file/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub(crate) fn check_file<'ctx>(
tracing::info_span!("Process check", path =? workspace_file.path.display()).in_scope(
move || {
if file_features.supports_lint() {
let lint_result = lint_with_guard(ctx, &mut workspace_file);
let lint_result = lint_with_guard(ctx, &mut workspace_file, false, None);
match lint_result {
Ok(status) => {
if status.is_changed() {
Expand Down
18 changes: 16 additions & 2 deletions crates/biome_cli/src/execute/process_file/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,21 @@ use std::path::Path;
use std::sync::atomic::Ordering;

/// Lints a single file and returns a [FileResult]
pub(crate) fn lint<'ctx>(ctx: &'ctx SharedTraversalOptions<'ctx, '_>, path: &Path) -> FileResult {
pub(crate) fn lint<'ctx>(
ctx: &'ctx SharedTraversalOptions<'ctx, '_>,
path: &Path,
suppress: bool,
suppression_reason: Option<&str>,
) -> FileResult {
let mut workspace_file = WorkspaceFile::new(ctx, path)?;
lint_with_guard(ctx, &mut workspace_file)
lint_with_guard(ctx, &mut workspace_file, suppress, suppression_reason)
}

pub(crate) fn lint_with_guard<'ctx>(
ctx: &'ctx SharedTraversalOptions<'ctx, '_>,
workspace_file: &mut WorkspaceFile,
suppress: bool,
suppression_reason: Option<&str>,
) -> FileResult {
tracing::info_span!("Processes linting", path =? workspace_file.path.display()).in_scope(
move || {
Expand All @@ -31,6 +38,12 @@ pub(crate) fn lint_with_guard<'ctx>(
(Vec::new(), Vec::new())
};
if let Some(fix_mode) = ctx.execution.as_fix_file_mode() {
let suppression_explanation = if suppress && suppression_reason.is_none() {
"ignored using `--suppress`"
} else {
suppression_reason.unwrap_or("<explanation>")
};

let fix_result = workspace_file
.guard()
.fix_file(
Expand All @@ -42,6 +55,7 @@ pub(crate) fn lint_with_guard<'ctx>(
.build(),
only.clone(),
skip.clone(),
Some(suppression_explanation.to_string()),
)
.with_file_path_and_code(
workspace_file.path.display().to_string(),
Expand Down
Loading

0 comments on commit 3f152b3

Please sign in to comment.