Skip to content

Commit

Permalink
Allow plugins to defined additional diagnostic notes for errors from …
Browse files Browse the repository at this point in the history
…plugin generated file (#6729)
  • Loading branch information
maciektr authored Nov 26, 2024
1 parent 0759c7a commit 040ee16
Show file tree
Hide file tree
Showing 41 changed files with 242 additions and 57 deletions.
27 changes: 21 additions & 6 deletions crates/cairo-lang-compiler/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ use std::fmt::Write;

use cairo_lang_defs::db::DefsGroup;
use cairo_lang_defs::ids::ModuleId;
use cairo_lang_diagnostics::{DiagnosticEntry, Diagnostics, FormattedDiagnosticEntry, Severity};
use cairo_lang_diagnostics::{
DiagnosticEntry, Diagnostics, FormattedDiagnosticEntry, PluginFileDiagnosticNotes, Severity,
};
use cairo_lang_filesystem::ids::{CrateId, FileLongId};
use cairo_lang_lowering::db::LoweringGroup;
use cairo_lang_parser::db::ParserGroup;
Expand Down Expand Up @@ -166,30 +168,42 @@ impl<'a> DiagnosticsReporter<'a> {
let modules = db.crate_modules(*crate_id);
let mut processed_file_ids = UnorderedHashSet::<_>::default();
for module_id in modules.iter() {
let diagnostic_notes =
db.module_plugin_diagnostics_notes(*module_id).unwrap_or_default();

if let Ok(module_files) = db.module_files(*module_id) {
for file_id in module_files.iter().copied() {
if processed_file_ids.insert(file_id) {
found_diagnostics |= self.check_diag_group(
db.upcast(),
db.file_syntax_diagnostics(file_id),
ignore_warnings_in_crate,
&diagnostic_notes,
);
}
}
}

if let Ok(group) = db.module_semantic_diagnostics(*module_id) {
found_diagnostics |=
self.check_diag_group(db.upcast(), group, ignore_warnings_in_crate);
found_diagnostics |= self.check_diag_group(
db.upcast(),
group,
ignore_warnings_in_crate,
&diagnostic_notes,
);
}

if self.skip_lowering_diagnostics {
continue;
}

if let Ok(group) = db.module_lowering_diagnostics(*module_id) {
found_diagnostics |=
self.check_diag_group(db.upcast(), group, ignore_warnings_in_crate);
found_diagnostics |= self.check_diag_group(
db.upcast(),
group,
ignore_warnings_in_crate,
&diagnostic_notes,
);
}
}
}
Expand All @@ -203,9 +217,10 @@ impl<'a> DiagnosticsReporter<'a> {
db: &TEntry::DbType,
group: Diagnostics<TEntry>,
skip_warnings: bool,
file_notes: &PluginFileDiagnosticNotes,
) -> bool {
let mut found: bool = false;
for entry in group.format_with_severity(db) {
for entry in group.format_with_severity(db, file_notes) {
if skip_warnings && entry.severity() == Severity::Warning {
continue;
}
Expand Down
35 changes: 33 additions & 2 deletions crates/cairo-lang-defs/src/db.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::collections::VecDeque;
use std::sync::Arc;

use cairo_lang_diagnostics::{Maybe, ToMaybe};
use cairo_lang_diagnostics::{DiagnosticNote, Maybe, PluginFileDiagnosticNotes, ToMaybe};
use cairo_lang_filesystem::db::FilesGroup;
use cairo_lang_filesystem::ids::{CrateId, Directory, FileId, FileKind, FileLongId, VirtualFile};
use cairo_lang_parser::db::ParserGroup;
Expand Down Expand Up @@ -253,6 +253,12 @@ pub trait DefsGroup:
&self,
module_id: ModuleId,
) -> Maybe<Arc<[(ModuleFileId, PluginDiagnostic)]>>;
/// Diagnostic notes for diagnostics originating in the plugin generated files identified by
/// [`FileId`].
fn module_plugin_diagnostics_notes(
&self,
module_id: ModuleId,
) -> Maybe<Arc<PluginFileDiagnosticNotes>>;
}

fn allowed_attributes(db: &dyn DefsGroup) -> Arc<OrderedHashSet<String>> {
Expand Down Expand Up @@ -410,6 +416,10 @@ pub struct ModuleData {
/// Generation info for each file. Virtual files have Some. Other files have None.
generated_file_aux_data: Vec<Option<DynGeneratedFileAuxData>>,
plugin_diagnostics: Vec<(ModuleFileId, PluginDiagnostic)>,
/// Diagnostic notes for diagnostics originating in the plugin generated files identified by
/// [`FileId`].
/// Diagnostic notes are added with `note: ` prefix at the end of diagnostic display.
diagnostics_notes: PluginFileDiagnosticNotes,
}

/// Information about generated files from running on a module file.
Expand All @@ -423,6 +433,10 @@ pub struct PrivModuleSubFiles {
items: Vec<ast::ModuleItem>,
/// The diagnostics generated by the plugins.
plugin_diagnostics: Vec<PluginDiagnostic>,
/// Diagnostic notes for diagnostics originating in the plugin generated files identified by
/// [`FileId`].
/// Diagnostic notes are added with `note: ` prefix at the end of diagnostic display.
diagnostics_notes: PluginFileDiagnosticNotes,
}

fn priv_module_data(db: &dyn DefsGroup, module_id: ModuleId) -> Maybe<ModuleData> {
Expand Down Expand Up @@ -467,6 +481,7 @@ fn priv_module_data(db: &dyn DefsGroup, module_id: ModuleId) -> Maybe<ModuleData
let mut aux_data = Vec::new();
let mut files = Vec::new();
let mut plugin_diagnostics = Vec::new();
let mut diagnostics_notes = OrderedHashMap::default();

let mut items = vec![];
aux_data.push(main_file_aux_data);
Expand All @@ -476,6 +491,7 @@ fn priv_module_data(db: &dyn DefsGroup, module_id: ModuleId) -> Maybe<ModuleData
files.push(file_id);

let priv_module_data = db.priv_module_sub_files(module_id, file_id)?;
diagnostics_notes.extend(priv_module_data.diagnostics_notes.clone().into_iter());
file_queue.extend(priv_module_data.files.keys().copied());
for diag in &priv_module_data.plugin_diagnostics {
plugin_diagnostics.push((module_file_id, diag.clone()));
Expand Down Expand Up @@ -588,6 +604,7 @@ fn priv_module_data(db: &dyn DefsGroup, module_id: ModuleId) -> Maybe<ModuleData
files,
generated_file_aux_data: aux_data,
plugin_diagnostics,
diagnostics_notes,
};
Ok(res)
}
Expand Down Expand Up @@ -653,6 +670,7 @@ fn priv_module_sub_files(
let mut aux_data = Vec::new();
let mut items = Vec::new();
let mut plugin_diagnostics = Vec::new();
let mut diagnostics_notes = OrderedHashMap::default();
for item_ast in item_asts.elements(syntax_db) {
let mut remove_original_item = false;
// Iterate the plugins by their order. The first one to change something (either
Expand All @@ -676,6 +694,10 @@ fn priv_module_sub_files(
.as_intern_id(),
)
.intern(db);
if let Some(text) = generated.diagnostics_note {
diagnostics_notes
.insert(generated_file_id, DiagnosticNote { text, location: None });
}
files.insert(generated_file_id, VirtualFile {
parent: Some(file_id),
name: generated.name,
Expand All @@ -696,7 +718,7 @@ fn priv_module_sub_files(
validate_attributes(syntax_db, &allowed_attributes, &item_ast, &mut plugin_diagnostics);
items.push(item_ast);
}
let res = PrivModuleSubFiles { files, aux_data, items, plugin_diagnostics };
let res = PrivModuleSubFiles { files, aux_data, items, plugin_diagnostics, diagnostics_notes };
Ok(res.into())
}

Expand Down Expand Up @@ -1077,6 +1099,15 @@ pub fn module_plugin_diagnostics(
Ok(db.priv_module_data(module_id)?.plugin_diagnostics.into())
}

/// Diagnostic notes for diagnostics originating in the plugin generated files identified by
/// [`FileId`].
pub fn module_plugin_diagnostics_notes(
db: &dyn DefsGroup,
module_id: ModuleId,
) -> Maybe<Arc<PluginFileDiagnosticNotes>> {
Ok(db.priv_module_data(module_id)?.diagnostics_notes.into())
}

fn module_items(db: &dyn DefsGroup, module_id: ModuleId) -> Maybe<Arc<[ModuleItemId]>> {
Ok(db.priv_module_data(module_id)?.items)
}
Expand Down
4 changes: 4 additions & 0 deletions crates/cairo-lang-defs/src/plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ pub struct PluginGeneratedFile {
pub code_mappings: Vec<CodeMapping>,
/// Arbitrary data that the plugin generates along with the file.
pub aux_data: Option<DynGeneratedFileAuxData>,
/// Diagnostic note for the plugin generated file.
/// This will be used as [`cairo_lang_diagnostics::DiagnosticNote`] on diagnostics originating
/// from this file.
pub diagnostics_note: Option<String>,
}

/// Result of plugin code generation.
Expand Down
3 changes: 3 additions & 0 deletions crates/cairo-lang-defs/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ impl MacroPlugin for DummyPlugin {
content: format!("fn f(x:{}){{}}", struct_ast.name(db).text(db)),
code_mappings: Default::default(),
aux_data: None,
diagnostics_note: Default::default(),
}),
diagnostics: vec![],
remove_original_item,
Expand All @@ -233,6 +234,7 @@ impl MacroPlugin for DummyPlugin {
content: "extern type B;".into(),
code_mappings: Default::default(),
aux_data: None,
diagnostics_note: Default::default(),
}),
diagnostics: vec![PluginDiagnostic::error(&free_function_ast, "bla".into())],
remove_original_item: false,
Expand Down Expand Up @@ -348,6 +350,7 @@ impl MacroPlugin for FooToBarPlugin {
content: "fn bar() {}".to_string(),
code_mappings: vec![],
aux_data: None,
diagnostics_note: Default::default(),
}),
diagnostics: vec![],
remove_original_item: false,
Expand Down
42 changes: 38 additions & 4 deletions crates/cairo-lang-diagnostics/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use cairo_lang_filesystem::db::{FilesGroup, get_originating_location};
use cairo_lang_filesystem::ids::FileId;
use cairo_lang_filesystem::span::TextSpan;
use cairo_lang_utils::Upcast;
use cairo_lang_utils::ordered_hash_map::OrderedHashMap;
use itertools::Itertools;

use crate::error_code::{ErrorCode, OptionErrorCodeExt};
Expand Down Expand Up @@ -53,6 +54,10 @@ pub trait DiagnosticEntry: Clone + fmt::Debug + Eq + Hash {
// TODO(spapini): Add a way to inspect the diagnostic programmatically, e.g, downcast.
}

/// Diagnostic notes for diagnostics originating in the plugin generated files identified by
/// [`FileId`].
pub type PluginFileDiagnosticNotes = OrderedHashMap<FileId, DiagnosticNote>;

// The representation of a source location inside a diagnostic.
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub struct DiagnosticLocation {
Expand All @@ -67,10 +72,29 @@ impl DiagnosticLocation {

/// Get the location of the originating user code.
pub fn user_location(&self, db: &dyn FilesGroup) -> Self {
let (file_id, span) = get_originating_location(db, self.file_id, self.span);
let (file_id, span) = get_originating_location(db, self.file_id, self.span, None);
Self { file_id, span }
}

/// Get the location of the originating user code,
/// along with [`DiagnosticNote`]s for this translation.
/// The notes are collected from the parent files of the originating location.
pub fn user_location_with_plugin_notes(
&self,
db: &dyn FilesGroup,
file_notes: &PluginFileDiagnosticNotes,
) -> (Self, Vec<DiagnosticNote>) {
let mut parent_files = Vec::new();
let (file_id, span) =
get_originating_location(db, self.file_id, self.span, Some(&mut parent_files));
let diagnostic_notes = parent_files
.into_iter()
.rev()
.filter_map(|file_id| file_notes.get(&file_id).cloned())
.collect_vec();
(Self { file_id, span }, diagnostic_notes)
}

/// Helper function to format the location of a diagnostic.
pub fn fmt_location(&self, f: &mut fmt::Formatter<'_>, db: &dyn FilesGroup) -> fmt::Result {
let user_location = self.user_location(db);
Expand Down Expand Up @@ -277,16 +301,26 @@ impl<TEntry: DiagnosticEntry> Diagnostics<TEntry> {
}

/// Format entries to pairs of severity and message.
pub fn format_with_severity(&self, db: &TEntry::DbType) -> Vec<FormattedDiagnosticEntry> {
pub fn format_with_severity(
&self,
db: &TEntry::DbType,
file_notes: &OrderedHashMap<FileId, DiagnosticNote>,
) -> Vec<FormattedDiagnosticEntry> {
let mut res: Vec<FormattedDiagnosticEntry> = Vec::new();

let files_db = db.upcast();
for entry in &self.get_diagnostics_without_duplicates(db) {
let mut msg = String::new();
msg += &format_diagnostics(files_db, &entry.format(db), entry.location(db));
let diag_location = entry.location(db);
let (user_location, parent_file_notes) =
diag_location.user_location_with_plugin_notes(files_db, file_notes);
msg += &format_diagnostics(files_db, &entry.format(db), user_location);
for note in entry.notes(db) {
msg += &format!("note: {:?}\n", note.debug(files_db))
}
for note in parent_file_notes {
msg += &format!("note: {:?}\n", note.debug(files_db))
}
msg += "\n";

let formatted =
Expand All @@ -298,7 +332,7 @@ impl<TEntry: DiagnosticEntry> Diagnostics<TEntry> {

/// Format entries to a [`String`] with messages prefixed by severity.
pub fn format(&self, db: &TEntry::DbType) -> String {
self.format_with_severity(db).iter().map(ToString::to_string).join("")
self.format_with_severity(db, &Default::default()).iter().map(ToString::to_string).join("")
}

/// Asserts that no diagnostic has occurred, panicking with an error message on failure.
Expand Down
4 changes: 2 additions & 2 deletions crates/cairo-lang-diagnostics/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

pub use diagnostics::{
DiagnosticAdded, DiagnosticEntry, DiagnosticLocation, DiagnosticNote, Diagnostics,
DiagnosticsBuilder, FormattedDiagnosticEntry, Maybe, Severity, ToMaybe, ToOption,
format_diagnostics, skip_diagnostic,
DiagnosticsBuilder, FormattedDiagnosticEntry, Maybe, PluginFileDiagnosticNotes, Severity,
ToMaybe, ToOption, format_diagnostics, skip_diagnostic,
};
pub use error_code::{ErrorCode, OptionErrorCodeExt};
pub use location_marks::get_location_marks;
Expand Down
7 changes: 7 additions & 0 deletions crates/cairo-lang-filesystem/src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,11 +343,18 @@ pub fn get_originating_location(
db: &dyn FilesGroup,
mut file_id: FileId,
mut span: TextSpan,
mut parent_files: Option<&mut Vec<FileId>>,
) -> (FileId, TextSpan) {
if let Some(ref mut parent_files) = parent_files {
parent_files.push(file_id);
}
while let Some((parent, code_mappings)) = get_parent_and_mapping(db, file_id) {
if let Some(origin) = code_mappings.iter().find_map(|mapping| mapping.translate(span)) {
span = origin;
file_id = parent;
if let Some(ref mut parent_files) = parent_files {
parent_files.push(file_id);
}
} else {
break;
}
Expand Down
4 changes: 3 additions & 1 deletion crates/cairo-lang-formatter/src/cairo_formatter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,9 @@ fn format_input(
db.file_content(file_id).ok_or_else(|| anyhow!("Unable to read from input."))?;
let (syntax_root, diagnostics) = get_syntax_root_and_diagnostics(&db, file_id, &original_text);
if diagnostics.check_error_free().is_err() {
return Err(FormattingError::ParsingError(diagnostics.format_with_severity(&db).into()));
return Err(FormattingError::ParsingError(
diagnostics.format_with_severity(&db, &Default::default()).into(),
));
}
let formatted_text = get_formatted_file(&db, &syntax_root, config.clone());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ fn get_definition_location(
let found_file = stable_ptr.file_id(syntax_db);
let span = node.span_without_trivia(syntax_db);
let width = span.width();
let (file_id, mut span) = get_originating_location(db.upcast(), found_file, span.start_only());
let (file_id, mut span) =
get_originating_location(db.upcast(), found_file, span.start_only(), None);
span.end = span.end.add_width(width);
Some((file_id, span))
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ pub fn inline_macro_generate_code(
}],
content,
aux_data: None,
diagnostics_note: Default::default(),
}),
diagnostics,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ impl InnerAttrExpansionContext {
content: expanded,
aux_data: None,
code_mappings,
diagnostics_note: Default::default(),
}),
diagnostics: self.diagnostics,
remove_original_item: true,
Expand Down Expand Up @@ -476,6 +477,7 @@ fn expand_derives(
}],
content,
aux_data: None,
diagnostics_note: Default::default(),
})
},
diagnostics: into_cairo_diagnostics(result.diagnostics, stable_ptr),
Expand Down Expand Up @@ -547,6 +549,7 @@ fn expand_attribute(
}],
content,
aux_data: None,
diagnostics_note: Default::default(),
}),
diagnostics: into_cairo_diagnostics(result.diagnostics, stable_ptr),
remove_original_item: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ Panics if any of the formatting of arguments fails.
```cairo
println!(); // Prints an empty line.
println!(\"hello\"); // Prints "hello".
let world: ByteArray = "world";
let world: ByteArray = "world";
println!("hello {}", world_ba); // Prints "hello world".
println!("hello {world_ba}"); // Prints "hello world".
```
Expand Down
Loading

0 comments on commit 040ee16

Please sign in to comment.