From 67c7979285c3a6c0b6f7053d436764e839d5576e Mon Sep 17 00:00:00 2001 From: "Victor M. Alvarez" Date: Wed, 18 Sep 2024 15:17:24 +0200 Subject: [PATCH] refactor: allow multiple footers in error/warning messages With this change we open the possibility of adding multiple footers to error messages. --- go/compiler.go | 14 +++++++- go/compiler_test.go | 30 ++++++++++++++++ lib/src/compiler/errors.rs | 14 ++++---- lib/src/compiler/mod.rs | 1 - lib/src/compiler/report.rs | 62 +++++++++++++++++++++++---------- lib/src/compiler/tests/mod.rs | 2 +- lib/src/compiler/warnings.rs | 8 ++--- macros/src/error.rs | 65 +++++++++++++++++++++++++++-------- macros/src/lib.rs | 2 +- 9 files changed, 150 insertions(+), 48 deletions(-) diff --git a/go/compiler.go b/go/compiler.go index 852215b8..33796830 100644 --- a/go/compiler.go +++ b/go/compiler.go @@ -121,6 +121,8 @@ type CompileError struct { Title string `json:"title"` // Each of the labels in the error report. Labels []Label `json:"labels"` + // Each of the footers in the error report. + Footers []Footer `json:"footers"` // The error's full report, as shown by the command-line tool. Text string `json:"text"` } @@ -131,8 +133,10 @@ type Warning struct { Code string `json:"code"` // Error title (e.g: "slow pattern"). Title string `json:"title"` - // Each of the labels in the error report. + // Each of the labels in the warning report. Labels []Label `json:"labels"` + // Each of the footers in the warning report. + Footers []Footer `json:"footers"` // The error's full report, as shown by the command-line tool. Text string `json:"text"` } @@ -149,6 +153,14 @@ type Label struct { Text string `json:"text"` } +// Footer represents a footer in a [CompileError]. +type Footer struct { + // Footer's level (e.g: "error", "warning", "info", "note", "help"). + Level string `json:"level"` + // Footer's text. + Text string `json:"text"` +} + // Span represents the starting and ending point of some piece of source // code. type Span struct { diff --git a/go/compiler_test.go b/go/compiler_test.go index 7c2fb6c5..5d624cee 100644 --- a/go/compiler_test.go +++ b/go/compiler_test.go @@ -131,6 +131,33 @@ func TestErrors(t *testing.T) { c.AddSource("rule test_1 { condition: true }") assert.Equal(t, []CompileError{}, c.Errors()) + assert.Equal(t, []Warning{ + { + Code: "invariant_expr", + Title: "invariant boolean expression", + Labels: []Label{ + { + Level: "warning", + Span: Span{Start: 25, End: 29}, + Text: "this expression is always true", + }, + }, + Footers: []Footer{ + { + Level: "note", + Text: "rule `test_1` is always `true`", + }, + }, + Text: `warning[invariant_expr]: invariant boolean expression + --> line:1:26 + | +1 | rule test_1 { condition: true } + | ---- this expression is always true + | + = note: rule ` + "`test_1` is always `true`", + }, + }, c.Warnings()) + c.AddSource("rule test_2 { condition: foo }", WithOrigin("test.yar")) assert.Equal(t, []CompileError{ { @@ -144,6 +171,7 @@ func TestErrors(t *testing.T) { Text: "this identifier has not been declared", }, }, + Footers: []Footer{}, Text: `error[E009]: unknown identifier ` + "`foo`" + ` --> test.yar:1:26 | @@ -241,6 +269,7 @@ func TestWarnings(t *testing.T) { Text: "these consecutive jumps will be treated as [0-2]", }, }, + Footers: []Footer{}, Text: `warning[consecutive_jumps]: consecutive jumps in hex pattern ` + "`$a`" + ` --> line:1:31 | @@ -259,6 +288,7 @@ func TestWarnings(t *testing.T) { Text: "this pattern may slow down the scan", }, }, + Footers: []Footer{}, Text: `warning[slow_pattern]: slow pattern --> line:1:22 | diff --git a/lib/src/compiler/errors.rs b/lib/src/compiler/errors.rs index a713fdc2..301f64db 100644 --- a/lib/src/compiler/errors.rs +++ b/lib/src/compiler/errors.rs @@ -10,7 +10,7 @@ use yara_x_macros::ErrorEnum; use yara_x_macros::ErrorStruct; use yara_x_parser::ast; -use crate::compiler::report::{Level, Report, ReportBuilder, CodeLoc, Label}; +use crate::compiler::report::{Level, Report, ReportBuilder, CodeLoc, Label, Footer}; /// Error returned while serializing/deserializing compiled rules. #[derive(Error, Debug)] @@ -203,7 +203,7 @@ pub struct MismatchingTypes { #[associated_enum(CompileError)] #[error(code = "E004", title = "wrong arguments")] #[label("wrong arguments in this call", error_loc)] -#[note(note)] +#[footer(note)] pub struct WrongArguments { report: Report, error_loc: CodeLoc, @@ -263,7 +263,7 @@ pub struct UnknownField { #[associated_enum(CompileError)] #[error(code = "E009", title = "unknown identifier `{identifier}`")] #[label("this identifier has not been declared", identifier_loc)] -#[note(note)] +#[footer(note)] pub struct UnknownIdentifier { report: Report, identifier: String, @@ -346,7 +346,7 @@ pub struct ConflictingRuleIdentifier { #[associated_enum(CompileError)] #[error(code = "E014", title = "invalid regular expression")] #[label("{error}", error_loc)] -#[note(note)] +#[footer(note)] pub struct InvalidRegexp { report: Report, error: String, @@ -376,7 +376,7 @@ pub struct MixedGreediness { #[associated_enum(CompileError)] #[error(code = "E016", title = "no matching patterns")] #[label("there's no pattern in this set", error_loc)] -#[note(note)] +#[footer(note)] pub struct EmptyPatternSet { report: Report, error_loc: CodeLoc, @@ -417,7 +417,7 @@ pub struct SlowPattern { )] #[label("`{modifier1}` modifier used here", modifier1_loc)] #[label("`{modifier2}` modifier used here", modifier2_loc)] -#[note(note)] +#[footer(note)] pub struct InvalidModifierCombination { report: Report, modifier1: String, @@ -481,7 +481,7 @@ pub struct DuplicatePattern { #[associated_enum(CompileError)] #[error(code = "E024", title = "invalid pattern `{pattern_ident}`")] #[label("{error}", error_loc)] -#[note(note)] +#[footer(note)] pub struct InvalidPattern { report: Report, pattern_ident: String, diff --git a/lib/src/compiler/mod.rs b/lib/src/compiler/mod.rs index 84b08cf8..5fdd3680 100644 --- a/lib/src/compiler/mod.rs +++ b/lib/src/compiler/mod.rs @@ -725,7 +725,6 @@ impl<'a> Compiler<'a> { }; rules.build_ac_automaton(); - rules } diff --git a/lib/src/compiler/report.rs b/lib/src/compiler/report.rs index e240b7e5..095d674f 100644 --- a/lib/src/compiler/report.rs +++ b/lib/src/compiler/report.rs @@ -92,7 +92,7 @@ pub(crate) struct Report { code: &'static str, title: String, labels: Vec<(Level, CodeLoc, String)>, - note: Option, + footers: Vec<(Level, String)>, } impl Report { @@ -112,22 +112,21 @@ impl Report { let code_origin = code_cache.get(&source_id).unwrap().origin.clone(); - let level = match level { - Level::Error => "error", - Level::Warning => "warning", - Level::Info => "info", - Level::Note => "note", - Level::Help => "help", - }; - - Label { level, code_origin, span: code_loc.span.clone(), text } + Label { + level: level_as_text(*level), + code_origin, + span: code_loc.span.clone(), + text, + } }) } - /// Returns the report's note. + /// Returns the report's footers. #[inline] - pub(crate) fn note(&self) -> Option<&str> { - self.note.as_deref() + pub(crate) fn footers(&self) -> impl Iterator { + self.footers + .iter() + .map(|(level, text)| Footer { level: level_as_text(*level), text }) } } @@ -140,7 +139,7 @@ impl Serialize for Report { s.serialize_field("code", &self.code)?; s.serialize_field("title", &self.title)?; s.serialize_field("labels", &self.labels().collect::>())?; - s.serialize_field("note", &self.note)?; + s.serialize_field("footers", &self.footers().collect::>())?; s.serialize_field("text", &self.to_string())?; s.end() } @@ -152,7 +151,7 @@ impl PartialEq for Report { && self.code.eq(other.code) && self.title.eq(&other.title) && self.labels.eq(&other.labels) - && self.note.eq(&other.note) + && self.footers.eq(&other.footers) } } @@ -210,8 +209,8 @@ impl Display for Report { message = message.snippet(snippet); - if let Some(note) = &self.note { - message = message.footer(Level::Note.title(note.as_str())); + for (level, text) in &self.footers { + message = message.footer(level.title(text.as_str())); } let renderer = if self.with_colors { @@ -236,6 +235,13 @@ pub struct Label<'a> { text: &'a str, } +/// Represents a footer in an error or warning report. +#[derive(Serialize)] +pub struct Footer<'a> { + level: &'a str, + text: &'a str, +} + /// Builds error and warning reports. /// /// `ReportBuilder` helps to create error and warning reports. It stores a copy @@ -373,11 +379,19 @@ impl ReportBuilder { code: &'static str, title: String, labels: Vec<(Level, CodeLoc, String)>, - note: Option, + footers: Vec<(Level, Option)>, ) -> Report { // Make sure there's at least one label. assert!(!labels.is_empty()); + // Remove footers where text is None. + let footers = footers + .into_iter() + .filter_map(|(level, text)| { + text.map(|text| (level, text)) + }) + .collect(); + Report { code_cache: self.code_cache.clone(), with_colors: self.with_colors, @@ -389,7 +403,17 @@ impl ReportBuilder { code, title, labels, - note, + footers, } } } + +fn level_as_text(level: Level) -> &'static str { + match level { + Level::Error => "error", + Level::Warning => "warning", + Level::Info => "info", + Level::Note => "note", + Level::Help => "help", + } +} diff --git a/lib/src/compiler/tests/mod.rs b/lib/src/compiler/tests/mod.rs index f11983e3..907e7e89 100644 --- a/lib/src/compiler/tests/mod.rs +++ b/lib/src/compiler/tests/mod.rs @@ -756,7 +756,7 @@ fn errors_serialization() { "text": "this identifier has not been declared" } ], - "note": null, + "footers": [], "text": r#"error[E009]: unknown identifier `foo` --> test.yar:1:23 | diff --git a/lib/src/compiler/warnings.rs b/lib/src/compiler/warnings.rs index 6c673bf2..a8085a25 100644 --- a/lib/src/compiler/warnings.rs +++ b/lib/src/compiler/warnings.rs @@ -8,7 +8,7 @@ use thiserror::Error; use yara_x_macros::ErrorEnum; use yara_x_macros::ErrorStruct; -use crate::compiler::report::{Level, Report, ReportBuilder, CodeLoc, Label}; +use crate::compiler::report::{Level, Report, ReportBuilder, CodeLoc, Label, Footer}; /// A warning raised while compiling YARA rules. #[allow(missing_docs)] @@ -169,7 +169,7 @@ pub struct PotentiallyUnsatisfiableExpression { "this expression is always {expr_value}", expr_loc )] -#[note(note)] +#[footer(note)] pub struct InvariantBooleanExpression { report: Report, expr_value: bool, @@ -200,7 +200,7 @@ pub struct InvariantBooleanExpression { "this expression is `{expr_type}` but is being used as `bool`", expr_loc )] -#[note(note)] +#[footer(note)] pub struct NonBooleanAsBoolean { report: Report, expr_type: String, @@ -383,7 +383,7 @@ pub struct SlowPattern { "module `{module_name}` used here", module_name_loc )] -#[note(note)] +#[footer(note)] pub struct IgnoredModule { report: Report, module_name: String, diff --git a/macros/src/error.rs b/macros/src/error.rs index 9e44728a..ea3ee169 100644 --- a/macros/src/error.rs +++ b/macros/src/error.rs @@ -16,7 +16,7 @@ struct Label { } impl Parse for Label { - /// Parses a label with like the one below. + /// Parses a label like the one below. /// /// ```text /// #[label("{error_msg}", error_ref, Level::Info)] @@ -36,6 +36,32 @@ impl Parse for Label { } } +/// Describes a footer in an error/warning message. +#[derive(Debug)] +struct Footer { + footer_expr: Expr, + level: Option, +} + +impl Parse for Footer { + /// Parses a footer like the one below. + /// + /// ```text + /// #[footer(text, Level::Info)] + /// ``` + /// + /// The last argument is optional, the default value is `Level::Note`. + fn parse(input: ParseStream) -> Result { + let footer_expr: Expr = input.parse()?; + let mut level = None; + if input.peek(Comma) { + input.parse::()?; + level = Some(input.parse::()?); + } + Ok(Footer { footer_expr, level }) + } +} + pub(crate) fn impl_error_struct_macro( input: DeriveInput, ) -> Result { @@ -52,9 +78,9 @@ pub(crate) fn impl_error_struct_macro( let mut level = None; let mut code = None; let mut title = None; - let mut note = None; let mut associated_enum = None; let mut labels = Vec::new(); + let mut footers = Vec::new(); for attr in input.attrs { if attr.path().is_ident("doc") { @@ -65,8 +91,8 @@ pub(crate) fn impl_error_struct_macro( associated_enum = Some(attr.parse_args::()?); } else if attr.path().is_ident("label") { labels.push(attr.parse_args::