-
Notifications
You must be signed in to change notification settings - Fork 87
Introduce machine-readable manifest diagnostics #3127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,8 @@ use toml::de::Error as TomlParseError; | |
| use scarb::core::Config; | ||
| use scarb::core::errors::{ManifestErrorWithSource, ManifestParseError}; | ||
| use scarb::core::{ | ||
| ManifestDiagnosticMessage, ManifestDiagnosticSpan, ManifestMessageKind, ManifestSemanticError, | ||
| MachineDiagnostic, MachineDiagnosticKind, MachineDiagnosticSeverity, MachineDiagnosticSpan, | ||
| MachineRelatedLocation, ManifestSemanticError, | ||
| }; | ||
| use scarb::ops; | ||
| use scarb_ui::OutputFormat; | ||
|
|
@@ -42,7 +43,7 @@ pub fn run(args: MetadataArgs, config: &Config) -> Result<()> { | |
| } | ||
|
|
||
| fn emit_manifest_diagnostic(config: &Config, error: &anyhow::Error) { | ||
| let (file, message, span, related) = if let Some(sem) = error | ||
| let diagnostic = if let Some(sem) = error | ||
| .chain() | ||
| .find_map(|c| c.downcast_ref::<ManifestSemanticError>()) | ||
| { | ||
|
|
@@ -59,7 +60,29 @@ fn emit_manifest_diagnostic(config: &Config, error: &anyhow::Error) { | |
| }) | ||
| .unwrap_or_default(); | ||
| let file = src.map(|src| src.path.to_string()); | ||
| (file, sem.to_string(), span, related) | ||
| let mut diagnostic = MachineDiagnostic::new( | ||
| MachineDiagnosticKind::ManifestDiagnostic, | ||
| sem.to_string(), | ||
| MachineDiagnosticSeverity::Error, | ||
| file.unwrap_or_else(|| "<unknown>".to_string()), | ||
| span.map(|span| MachineDiagnosticSpan { | ||
| start: span.start, | ||
| end: span.end, | ||
| }) | ||
| .unwrap_or(MachineDiagnosticSpan { start: 0, end: 0 }), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be default for MachineDiagnosticSpan?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, if the span is no longer an |
||
| ); | ||
| diagnostic.related = related | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: maybe use a builder pattern instead? could get rid of setting the defaults too. |
||
| .into_iter() | ||
| .map(|related| MachineRelatedLocation { | ||
| message: related.message, | ||
| file: None, | ||
| span: MachineDiagnosticSpan { | ||
| start: related.span.start, | ||
| end: related.span.end, | ||
| }, | ||
| }) | ||
| .collect(); | ||
| diagnostic | ||
| } else if let Some(parse_err) = error | ||
| .chain() | ||
| .find_map(|c| c.downcast_ref::<ManifestParseError>()) | ||
|
|
@@ -76,11 +99,21 @@ fn emit_manifest_diagnostic(config: &Config, error: &anyhow::Error) { | |
| }; | ||
| let span = toml_err | ||
| .and_then(|e| e.span()) | ||
| .map(|s| ManifestDiagnosticSpan { | ||
| .map(|s| MachineDiagnosticSpan { | ||
| start: s.start, | ||
| end: s.end, | ||
| }); | ||
| (Some(parse_err.path().to_string()), message, span, vec![]) | ||
| MachineDiagnostic::new( | ||
| MachineDiagnosticKind::ManifestDiagnostic, | ||
| message, | ||
| MachineDiagnosticSeverity::Error, | ||
| parse_err.path().to_string(), | ||
| span.map(|span| MachineDiagnosticSpan { | ||
| start: span.start, | ||
| end: span.end, | ||
| }) | ||
| .unwrap_or(MachineDiagnosticSpan { start: 0, end: 0 }), | ||
| ) | ||
| } else if let Some(src) = error | ||
| .chain() | ||
| .find_map(|c| c.downcast_ref::<ManifestErrorWithSource>()) | ||
|
|
@@ -89,18 +122,16 @@ fn emit_manifest_diagnostic(config: &Config, error: &anyhow::Error) { | |
| .source() | ||
| .map(|err| err.to_string()) | ||
| .unwrap_or_else(|| error.to_string()); | ||
| (Some(src.path.to_string()), message, None, vec![]) | ||
| MachineDiagnostic::new( | ||
| MachineDiagnosticKind::ManifestDiagnostic, | ||
| message, | ||
| MachineDiagnosticSeverity::Error, | ||
| src.path.to_string(), | ||
| MachineDiagnosticSpan { start: 0, end: 0 }, | ||
| ) | ||
| } else { | ||
| return; | ||
| }; | ||
|
|
||
| config | ||
| .ui() | ||
| .force_print(MachineMessage(ManifestDiagnosticMessage { | ||
| kind: ManifestMessageKind::ManifestDiagnostic, | ||
| message, | ||
| file, | ||
| span, | ||
| related, | ||
| })); | ||
| config.ui().force_print(MachineMessage(diagnostic)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: could be just Message impl for MachineDiagnostic. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ use cairo_lang_compiler::CompilerConfig; | |
| use cairo_lang_compiler::diagnostics::DiagnosticsReporter; | ||
| use cairo_lang_diagnostics::{FormattedDiagnosticEntry, Severity}; | ||
| use cairo_lang_filesystem::db::FilesGroup; | ||
| use cairo_lang_filesystem::ids::CrateId; | ||
| use cairo_lang_filesystem::ids::{CrateId, CrateInput}; | ||
| use itertools::Itertools; | ||
| use salsa::Database; | ||
| use serde::Serialize; | ||
|
|
@@ -41,6 +41,13 @@ impl<W: Write> Write for CountingWriter<W> { | |
| } | ||
| } | ||
|
|
||
| pub fn all_crate_inputs(db: &dyn Database) -> Vec<CrateInput> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is this used? |
||
| db.crates() | ||
| .iter() | ||
| .map(|crate_id| crate_id.long(db).clone().into_crate_input(db)) | ||
| .collect_vec() | ||
| } | ||
|
|
||
| pub fn build_compiler_config<'c, 'db>( | ||
| db: &'db dyn Database, | ||
| unit: &CairoCompilationUnit, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,83 @@ | ||||||
| use serde::Serialize; | ||||||
|
|
||||||
| #[derive(Debug, Clone, Serialize)] | ||||||
| pub struct MachineDiagnostic { | ||||||
| pub kind: MachineDiagnosticKind, | ||||||
| pub message: String, | ||||||
| pub file: String, | ||||||
| pub span: MachineDiagnosticSpan, | ||||||
| pub severity: MachineDiagnosticSeverity, | ||||||
| #[serde(skip_serializing_if = "Option::is_none")] | ||||||
| pub code: Option<String>, | ||||||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||||||
| pub related: Vec<MachineRelatedLocation>, | ||||||
| } | ||||||
|
|
||||||
| #[derive(Debug, Clone, Copy, Serialize, PartialEq, Eq)] | ||||||
| #[serde(rename_all = "snake_case")] | ||||||
| pub enum MachineDiagnosticKind { | ||||||
| Diagnostic, | ||||||
| ManifestDiagnostic, | ||||||
| } | ||||||
|
|
||||||
| #[derive(Debug, Clone, Copy, Serialize, PartialEq, Eq)] | ||||||
| #[serde(rename_all = "snake_case")] | ||||||
| pub enum MachineDiagnosticSeverity { | ||||||
| Error, | ||||||
| Warning, | ||||||
| } | ||||||
|
|
||||||
| #[derive(Debug, Clone, Serialize)] | ||||||
| pub struct MachineDiagnosticSpan { | ||||||
| pub start: usize, | ||||||
| pub end: usize, | ||||||
| } | ||||||
|
|
||||||
| #[derive(Debug, Clone, Serialize)] | ||||||
| pub struct MachineRelatedLocation { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit
Suggested change
|
||||||
| pub message: String, | ||||||
| #[serde(skip_serializing_if = "Option::is_none")] | ||||||
| pub file: Option<String>, | ||||||
| pub span: MachineDiagnosticSpan, | ||||||
| } | ||||||
|
|
||||||
| #[derive(Debug, Clone, Serialize)] | ||||||
| pub struct MachineDiagnosticData { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this used anywhere? |
||||||
| #[serde(skip_serializing_if = "Option::is_none")] | ||||||
| pub span: Option<MachineDiagnosticSpan>, | ||||||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||||||
| pub related: Vec<MachineRelatedLocation>, | ||||||
| } | ||||||
|
|
||||||
| impl MachineDiagnostic { | ||||||
| pub fn new( | ||||||
| kind: MachineDiagnosticKind, | ||||||
| message: String, | ||||||
| severity: MachineDiagnosticSeverity, | ||||||
| file: String, | ||||||
| span: MachineDiagnosticSpan, | ||||||
| ) -> Self { | ||||||
| Self { | ||||||
| kind, | ||||||
| message, | ||||||
| severity, | ||||||
| code: None, | ||||||
| file, | ||||||
| span, | ||||||
| related: vec![], | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| pub fn severity(&self) -> MachineDiagnosticSeverity { | ||||||
| self.severity | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| impl From<std::ops::Range<usize>> for MachineDiagnosticSpan { | ||||||
| fn from(range: std::ops::Range<usize>) -> Self { | ||||||
| Self { | ||||||
| start: range.start, | ||||||
| end: range.end, | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i'd assign the
<unknown>string to some consts and have it named properly, so its clear it describes "no file" state