From 713f29a630bd212d4ae49f8f4db6bef279647e7e Mon Sep 17 00:00:00 2001 From: Chris Rybicki Date: Thu, 5 Oct 2023 09:48:16 -0400 Subject: [PATCH] feat(compiler): error diagnostics v2 (#4416) The Wing compiler now has the capability to provide more detailed information with each compiler error. I've implemented a few examples in this pull request to demonstrate the feature: Example 1: showing the original symbol definition in "use before defined" errors: Screenshot 2023-10-04 at 6 34 41 PM Example 2: showing the previous definition in "symbol already defined" errors: Screenshot 2023-10-04 at 6 36 01 PM Example 3: showing the variable definition in "variable is not reassignable" errors: Screenshot 2023-10-04 at 6 31 04 PM These errors are also displayed as related information in the LSP with clickable links: Screenshot 2023-10-04 at 6 21 01 PM I haven't tested what happens when the annotations refer to other files, but in theory it should also work... ## Checklist - [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted) - [x] Description explains motivation and solution - [x] Tests added (always) - [ ] Docs updated (only required for features) - [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing *By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*. --- .../server/src/utils/format-wing-error.ts | 26 ++++- apps/wing/src/commands/compile.ts | 24 +++- apps/wing/src/commands/lsp.ts | 13 ++- libs/wingc/src/comp_ctx.rs | 1 + libs/wingc/src/diagnostic.rs | 8 ++ libs/wingc/src/files.rs | 1 + libs/wingc/src/jsify.rs | 2 + libs/wingc/src/json_schema_generator.rs | 2 +- libs/wingc/src/lib.rs | 3 + libs/wingc/src/lifting.rs | 5 +- libs/wingc/src/lsp/completions.rs | 4 +- libs/wingc/src/parser.rs | 6 + libs/wingc/src/type_check.rs | 107 +++++++++++------- libs/wingc/src/type_check/symbol_env.rs | 30 +++-- libs/wingc/src/valid_json_visitor.rs | 3 + libs/wingc/src/visit_types.rs | 12 +- libs/wingcompiler/src/wingc.ts | 16 ++- tools/hangar/__snapshots__/invalid.ts.snap | 27 +++++ 18 files changed, 225 insertions(+), 65 deletions(-) diff --git a/apps/wing-console/console/server/src/utils/format-wing-error.ts b/apps/wing-console/console/server/src/utils/format-wing-error.ts index b81cd3aed1a..9866572953c 100644 --- a/apps/wing-console/console/server/src/utils/format-wing-error.ts +++ b/apps/wing-console/console/server/src/utils/format-wing-error.ts @@ -42,12 +42,12 @@ export const formatWingError = async (error: unknown) => { const result = []; for (const error of errors) { - const { message, span } = error; + const { message, span, annotations } = error; let files: File[] = []; let labels: Label[] = []; // file_id might be "" if the span is synthetic (see #2521) - if (span !== null && span.file_id) { + if (span !== null && span !== undefined && span.file_id) { // `span` should only be null if source file couldn't be read etc. const source = await readFile(span.file_id, "utf8"); const start = offsetFromLineAndColumn( @@ -70,6 +70,28 @@ export const formatWingError = async (error: unknown) => { }); } + for (const annotation of annotations) { + const source = await readFile(annotation.span.file_id, "utf8"); + const start = offsetFromLineAndColumn( + source, + annotation.span.start.line, + annotation.span.start.col, + ); + const end = offsetFromLineAndColumn( + source, + annotation.span.end.line, + annotation.span.end.col, + ); + files.push({ name: annotation.span.file_id, source }); + labels.push({ + fileId: annotation.span.file_id, + rangeStart: start, + rangeEnd: end, + message: annotation.message, + style: "secondary", + }); + } + console.log("pre diagnostic"); const diagnosticText = emitDiagnostic( files, diff --git a/apps/wing/src/commands/compile.ts b/apps/wing/src/commands/compile.ts index 28e5f91f98f..30b2512ef3a 100644 --- a/apps/wing/src/commands/compile.ts +++ b/apps/wing/src/commands/compile.ts @@ -54,7 +54,7 @@ export async function compile(entrypoint: string, options: CompileOptions): Prom const result = []; for (const diagnostic of diagnostics) { - const { message, span } = diagnostic; + const { message, span, annotations } = diagnostic; let files: File[] = []; let labels: Label[] = []; @@ -74,6 +74,28 @@ export async function compile(entrypoint: string, options: CompileOptions): Prom }); } + for (const annotation of annotations) { + const source = await fsPromise.readFile(annotation.span.file_id, "utf8"); + const start = byteOffsetFromLineAndColumn( + source, + annotation.span.start.line, + annotation.span.start.col + ); + const end = byteOffsetFromLineAndColumn( + source, + annotation.span.end.line, + annotation.span.end.col + ); + files.push({ name: annotation.span.file_id, source }); + labels.push({ + fileId: annotation.span.file_id, + rangeStart: start, + rangeEnd: end, + message: annotation.message, + style: "secondary", + }); + } + const diagnosticText = emitDiagnostic( files, { diff --git a/apps/wing/src/commands/lsp.ts b/apps/wing/src/commands/lsp.ts index f7c12b7d304..2fdf733478e 100644 --- a/apps/wing/src/commands/lsp.ts +++ b/apps/wing/src/commands/lsp.ts @@ -8,6 +8,7 @@ import { Diagnostic, Range, DocumentUri, + Location, } from "vscode-languageserver/node"; export async function lsp() { @@ -130,7 +131,17 @@ export async function lsp() { const diagnosticUri = "file://" + rd.span.file_id; const diag = Diagnostic.create( Range.create(rd.span.start.line, rd.span.start.col, rd.span.end.line, rd.span.end.col), - rd.message + rd.message, + undefined, + undefined, + undefined, + rd.annotations.map((a) => ({ + location: Location.create( + "file://" + a.span.file_id, + Range.create(a.span.start.line, a.span.start.col, a.span.end.line, a.span.end.col) + ), + message: a.message, + })) ); if (!allDiagnostics.has(diagnosticUri)) { diff --git a/libs/wingc/src/comp_ctx.rs b/libs/wingc/src/comp_ctx.rs index d18ea3c7a69..d883181f0b6 100644 --- a/libs/wingc/src/comp_ctx.rs +++ b/libs/wingc/src/comp_ctx.rs @@ -110,6 +110,7 @@ pub fn set_custom_panic_hook() { CompilationContext::get_phase() ), span: Some(CompilationContext::get_span()), + annotations: vec![], }) })); } diff --git a/libs/wingc/src/diagnostic.rs b/libs/wingc/src/diagnostic.rs index e95b98e86b7..10ac8b508ab 100644 --- a/libs/wingc/src/diagnostic.rs +++ b/libs/wingc/src/diagnostic.rs @@ -187,9 +187,16 @@ impl PartialOrd for WingSpan { #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] pub struct Diagnostic { pub message: String, + pub annotations: Vec, pub span: Option, } +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] +pub struct DiagnosticAnnotation { + pub message: String, + pub span: WingSpan, +} + impl std::fmt::Display for Diagnostic { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { if let Some(span) = &self.span { @@ -283,6 +290,7 @@ pub fn reset_diagnostics() { pub struct TypeError { pub message: String, pub span: WingSpan, + pub annotations: Vec, } impl std::fmt::Display for TypeError { diff --git a/libs/wingc/src/files.rs b/libs/wingc/src/files.rs index 8b4877d8fc9..3ff360c6676 100644 --- a/libs/wingc/src/files.rs +++ b/libs/wingc/src/files.rs @@ -30,6 +30,7 @@ impl From for Diagnostic { Self { message: err.to_string(), span: None, + annotations: vec![], } } } diff --git a/libs/wingc/src/jsify.rs b/libs/wingc/src/jsify.rs index 0dd56c97d21..f0fe6079323 100644 --- a/libs/wingc/src/jsify.rs +++ b/libs/wingc/src/jsify.rs @@ -404,6 +404,7 @@ impl<'a> JSifier<'a> { report_diagnostic(Diagnostic { message: "Cannot reference an inflight value from within a preflight expression".to_string(), span: Some(expression.span.clone()), + annotations: vec![], }); return "".to_string(); @@ -1089,6 +1090,7 @@ impl<'a> JSifier<'a> { report_diagnostic(Diagnostic { message: format!("Failed to resolve extern \"{external_spec}\": {err}"), span: Some(func_def.span.clone()), + annotations: vec![], }); format!("/* unresolved: \"{external_spec}\" */") } diff --git a/libs/wingc/src/json_schema_generator.rs b/libs/wingc/src/json_schema_generator.rs index c1a2511cfed..80b7cd78c71 100644 --- a/libs/wingc/src/json_schema_generator.rs +++ b/libs/wingc/src/json_schema_generator.rs @@ -25,7 +25,7 @@ impl JsonSchemaGenerator { fn get_struct_schema_required_fields(&self, env: &SymbolEnv) -> CodeMaker { let mut code = CodeMaker::default(); code.open("required: ["); - for (field_name, (_stmt_idx, kind)) in env.symbol_map.iter() { + for (field_name, (_stmt_idx, _, kind)) in env.symbol_map.iter() { if !matches!(*kind.as_variable().unwrap().type_, Type::Optional(_)) { code.line(format!("\"{}\",", field_name)); } diff --git a/libs/wingc/src/lib.rs b/libs/wingc/src/lib.rs index 3043585cccf..bb2a8fe2057 100644 --- a/libs/wingc/src/lib.rs +++ b/libs/wingc/src/lib.rs @@ -165,6 +165,7 @@ pub unsafe extern "C" fn wingc_compile(ptr: u32, len: u32) -> u64 { report_diagnostic(Diagnostic { message: format!("Expected 3 arguments to wingc_compile, got {}", split.len()), span: None, + annotations: vec![], }); return WASM_RETURN_ERROR; } @@ -176,6 +177,7 @@ pub unsafe extern "C" fn wingc_compile(ptr: u32, len: u32) -> u64 { report_diagnostic(Diagnostic { message: format!("Source path cannot be found: {}", source_path), span: None, + annotations: vec![], }); return WASM_RETURN_ERROR; } @@ -338,6 +340,7 @@ pub fn compile( report_diagnostic(Diagnostic { message: format!("Project directory must be absolute: {}", project_dir), span: None, + annotations: vec![], }); return Err(()); } diff --git a/libs/wingc/src/lifting.rs b/libs/wingc/src/lifting.rs index ec5a9be6c41..8378994a433 100644 --- a/libs/wingc/src/lifting.rs +++ b/libs/wingc/src/lifting.rs @@ -56,11 +56,12 @@ impl<'a> LiftVisitor<'a> { if let Some(env) = self.ctx.current_env() { if matches!( env.lookup_ext(symbol, Some(self.ctx.current_stmt_idx())), - LookupResult::DefinedLater + LookupResult::DefinedLater(_) ) { report_diagnostic(Diagnostic { span: Some(symbol.span.clone()), message: format!("Cannot access \"{symbol}\" because it is shadowed by another symbol with the same name"), + annotations: vec![], }); } } @@ -215,6 +216,7 @@ impl<'a> Visit<'a> for LiftVisitor<'a> { expr_type.to_string() ), span: Some(node.span.clone()), + annotations: vec![], }); return; @@ -279,6 +281,7 @@ impl<'a> Visit<'a> for LiftVisitor<'a> { message: format!( "Cannot qualify access to a lifted type \"{udt_type}\" (see https://github.com/winglang/wing/issues/76 for more details)"), span: Some(node.span.clone()), + annotations: vec![], }); return; diff --git a/libs/wingc/src/lsp/completions.rs b/libs/wingc/src/lsp/completions.rs index 7e5b6b3d1b3..5e94484896d 100644 --- a/libs/wingc/src/lsp/completions.rs +++ b/libs/wingc/src/lsp/completions.rs @@ -510,7 +510,7 @@ fn get_current_scope_completions( true } }) { - let symbol_kind = &symbol_data.1 .1; + let symbol_kind = &symbol_data.1 .2; if let Some(completion) = format_symbol_kind_as_completion(symbol_data.0, symbol_kind) { completions.push(completion); @@ -731,7 +731,7 @@ fn get_completions_from_namespace( .envs .iter() .flat_map(|env| env.symbol_map.iter()) - .flat_map(|(name, symbol)| format_symbol_kind_as_completion(name, &symbol.1)) + .flat_map(|(name, symbol)| format_symbol_kind_as_completion(name, &symbol.2)) .chain(util_completions.into_iter()) .collect() } diff --git a/libs/wingc/src/parser.rs b/libs/wingc/src/parser.rs index a310905a029..3c4d38ed91a 100644 --- a/libs/wingc/src/parser.rs +++ b/libs/wingc/src/parser.rs @@ -203,6 +203,7 @@ pub fn parse_wing_project( formatted_cycle.trim_end() ), span: None, + annotations: vec![], }); // return a list of all files just so we can continue type-checking @@ -359,6 +360,7 @@ impl<'s> Parser<'s> { let diag = Diagnostic { message: message.to_string(), span: Some(span), + annotations: vec![], }; report_diagnostic(diag); } @@ -367,6 +369,7 @@ impl<'s> Parser<'s> { let diag = Diagnostic { message: message.to_string(), span: Some(self.node_span(node)), + annotations: vec![], }; report_diagnostic(diag); @@ -996,6 +999,7 @@ impl<'s> Parser<'s> { message: "Static class fields not supported yet, see https://github.com/winglang/wing/issues/1668" .to_string(), span: Some(self.node_span(&class_element)), + annotations: vec![], }); } @@ -2115,6 +2119,7 @@ impl<'s> Parser<'s> { let diag = Diagnostic { message: "Expected ';'".to_string(), span: Some(self.node_span(&target_node)), + annotations: vec![], }; report_diagnostic(diag); } else if node.kind() == "AUTOMATIC_BLOCK" { @@ -2135,6 +2140,7 @@ impl<'s> Parser<'s> { let diag = Diagnostic { message: format!("Expected '{}'", node.kind()), span: Some(self.node_span(&target_node)), + annotations: vec![], }; report_diagnostic(diag); } diff --git a/libs/wingc/src/type_check.rs b/libs/wingc/src/type_check.rs index b74dd8ba3ef..12e85e83471 100644 --- a/libs/wingc/src/type_check.rs +++ b/libs/wingc/src/type_check.rs @@ -14,7 +14,7 @@ use crate::ast::{ TypeAnnotation, UnaryOperator, UserDefinedType, }; use crate::comp_ctx::{CompilationContext, CompilationPhase}; -use crate::diagnostic::{report_diagnostic, Diagnostic, TypeError, WingSpan}; +use crate::diagnostic::{report_diagnostic, Diagnostic, DiagnosticAnnotation, TypeError, WingSpan}; use crate::docs::Docs; use crate::file_graph::FileGraph; use crate::type_check::symbol_env::SymbolEnvKind; @@ -81,7 +81,7 @@ pub type TypeRef = UnsafeRef; #[derive(Debug)] pub enum SymbolKind { - Type(TypeRef), // TODO: <- deprecated since we treat types as a VeriableInfo of kind VariableKind::Type + Type(TypeRef), // TODO: <- deprecated since we treat types as a VariableInfo of kind VariableKind::Type Variable(VariableInfo), Namespace(NamespaceRef), } @@ -1458,6 +1458,7 @@ impl Types { report_diagnostic(Diagnostic { message: format!("Inferred type {new_type} conflicts with already inferred type {existing_type}"), span: Some(span.clone()), + annotations: vec![], }); existing_type_option.replace(error); return; @@ -1739,6 +1740,7 @@ impl<'a> TypeChecker<'a> { report_diagnostic(Diagnostic { message: message.into(), span: Some(spanned.span()), + annotations: vec![], }); (self.make_error_variable_info(), Phase::Independent) @@ -1748,6 +1750,7 @@ impl<'a> TypeChecker<'a> { report_diagnostic(Diagnostic { message: message.into(), span: Some(spanned.span()), + annotations: vec![], }); } @@ -1755,14 +1758,20 @@ impl<'a> TypeChecker<'a> { report_diagnostic(Diagnostic { message: message.into(), span: None, + annotations: vec![], }); } fn type_error(&self, type_error: TypeError) -> TypeRef { - let TypeError { message, span } = type_error; + let TypeError { + message, + span, + annotations, + } = type_error; report_diagnostic(Diagnostic { message, span: Some(span), + annotations, }); self.types.error() @@ -2484,6 +2493,7 @@ impl<'a> TypeChecker<'a> { self.type_error(TypeError { message: "Panic expression".to_string(), span: exp.span.clone(), + annotations: vec![], }), env.phase, ) @@ -2901,10 +2911,7 @@ impl<'a> TypeChecker<'a> { // known json data is statically known message = format!("{message} (hint: use {first_expected_type}.fromJson() to convert dynamic Json)"); } - report_diagnostic(Diagnostic { - message, - span: Some(span.span()), - }); + self.spanned_error(span, message); // Evaluate to one of the expected types first_expected_type @@ -2965,6 +2972,7 @@ impl<'a> TypeChecker<'a> { SymbolEnvOrNamespace::Error(Diagnostic { message: format!("Could not bring \"{}\" due to cyclic bring statements", source_path,), span: None, + annotations: vec![], }), ); return; @@ -2987,6 +2995,7 @@ impl<'a> TypeChecker<'a> { SymbolEnvOrNamespace::Error(Diagnostic { message: format!("Symbol \"{}\" has multiple definitions in \"{}\"", key, source_path), span: None, + annotations: vec![], }), ); return; @@ -3035,7 +3044,7 @@ impl<'a> TypeChecker<'a> { } for symbol_data in env.symbol_map.values_mut() { - if let SymbolKind::Variable(ref mut var_info) = symbol_data.1 { + if let SymbolKind::Variable(ref mut var_info) = symbol_data.2 { // Update any possible inferred types in this variable. // This must be called before checking for un-inferred types because some variable were not used in this scope so they did not get a chance to get updated. self.update_known_inferences(&mut var_info.type_, &var_info.name.span); @@ -3879,7 +3888,14 @@ impl<'a> TypeChecker<'a> { let (var, var_phase) = self.resolve_reference(&variable, env); if !var.type_.is_unresolved() && !var.reassignable { - self.spanned_error(variable, "Variable is not reassignable".to_string()); + report_diagnostic(Diagnostic { + message: "Variable is not reassignable".to_string(), + span: Some(variable.span()), + annotations: vec![DiagnosticAnnotation { + message: "defined here (try adding \"var\" in front)".to_string(), + span: var.name.span(), + }], + }); } else if var_phase == Phase::Preflight && env.phase == Phase::Inflight { self.spanned_error(variable, "Variable cannot be reassigned from inflight".to_string()); } @@ -4084,10 +4100,10 @@ impl<'a> TypeChecker<'a> { } if !cond_type.is_option() { - report_diagnostic(Diagnostic { - message: format!("Expected type to be optional, but got \"{}\" instead", cond_type), - span: Some(value.span()), - }); + self.spanned_error( + value, + format!("Expected type to be optional, but got \"{}\" instead", cond_type), + ) } // Technically we only allow if let statements to be used with optionals @@ -5248,20 +5264,17 @@ impl<'a> TypeChecker<'a> { if parent_class.phase == phase { (Some(parent_type), Some(parent_class.env.get_ref())) } else { - report_diagnostic(Diagnostic { - message: format!( + self.spanned_error( + parent, + format!( "Class \"{}\" is an {} class and cannot extend {} class \"{}\"", name, phase, parent_class.phase, parent_class.name ), - span: Some(parent.span.clone()), - }); + ); (None, None) } } else { - report_diagnostic(Diagnostic { - message: format!("Expected \"{}\" to be a class", parent), - span: Some(parent.span.clone()), - }); + self.spanned_error(parent, format!("Expected \"{}\" to be a class", parent)); (None, None) } } @@ -5289,6 +5302,7 @@ fn add_parent_members_to_struct_env( name.name, parent_type ), span: name.span.clone(), + annotations: vec![], }); }; // Add each member of current parent to the struct's environment (if it wasn't already added by a previous parent) @@ -5309,6 +5323,7 @@ fn add_parent_members_to_struct_env( "Struct \"{}\" extends \"{}\" which introduces a conflicting member \"{}\" ({} != {})", name, parent_type, parent_member_name, member_type, member_type ), + annotations: vec![], }); } } else { @@ -5352,6 +5367,7 @@ fn add_parent_members_to_iface_env( name.name, parent_type ), span: name.span.clone(), + annotations: vec![], }); }; // Add each member of current parent to the interface's environment (if it wasn't already added by a previous parent) @@ -5367,11 +5383,12 @@ fn add_parent_members_to_iface_env( .type_; if !existing_type.is_same_type_as(&member_type) { return Err(TypeError { - span: name.span.clone(), message: format!( "Interface \"{}\" extends \"{}\" but has a conflicting member \"{}\" ({} != {})", name, parent_type, parent_member_name, member_type, member_type ), + span: name.span.clone(), + annotations: vec![], }); } } else { @@ -5407,23 +5424,32 @@ fn lookup_result_to_type_error(lookup_result: LookupResult, looked_up_object: where T: Spanned + Display, { - let (message, span) = match lookup_result { - LookupResult::NotFound(s) => (format!("Unknown symbol \"{s}\""), s.span()), - LookupResult::MultipleFound => ( - format!("Ambiguous symbol \"{looked_up_object}\""), - looked_up_object.span(), - ), - LookupResult::DefinedLater => ( - format!("Symbol \"{looked_up_object}\" used before being defined"), - looked_up_object.span(), - ), - LookupResult::ExpectedNamespace(ns_name) => ( - format!("Expected \"{ns_name}\" in \"{looked_up_object}\" to be a namespace"), - ns_name.span(), - ), + match lookup_result { + LookupResult::NotFound(s) => TypeError { + message: format!("Unknown symbol \"{s}\""), + span: s.span(), + annotations: vec![], + }, + LookupResult::MultipleFound => TypeError { + message: format!("Ambiguous symbol \"{looked_up_object}\""), + span: looked_up_object.span(), + annotations: vec![], + }, + LookupResult::DefinedLater(span) => TypeError { + message: format!("Symbol \"{looked_up_object}\" used before being defined"), + span: looked_up_object.span(), + annotations: vec![DiagnosticAnnotation { + message: "defined later here".to_string(), + span, + }], + }, + LookupResult::ExpectedNamespace(ns_name) => TypeError { + message: format!("Expected \"{ns_name}\" in \"{looked_up_object}\" to be a namespace"), + span: ns_name.span(), + annotations: vec![], + }, LookupResult::Found(..) => panic!("Expected a lookup error, but found a successful lookup"), - }; - TypeError { message, span } + } } /// Resolves a user defined type (e.g. `Foo.Bar.Baz`) to a type reference @@ -5446,6 +5472,7 @@ pub fn resolve_user_defined_type( Err(TypeError { message: format!("Expected \"{}\" to be a type but it's a {symb_kind}", symb.name), span: symb.span.clone(), + annotations: vec![], }) } } else { @@ -5475,6 +5502,7 @@ pub fn resolve_super_method(method: &Symbol, env: &SymbolEnv, types: &Types) -> "`super` calls inside inflight closures not supported yet, see: https://github.com/winglang/wing/issues/3474" .to_string(), span: method.span.clone(), + annotations: vec![], }); } // Get the parent type of "this" (if it's a preflight class that's directly derived from `std.Resource` it's an implicit derive so we'll treat it as if there's no parent) @@ -5493,12 +5521,14 @@ pub fn resolve_super_method(method: &Symbol, env: &SymbolEnv, types: &Types) -> parent_type, method ), span: method.span.clone(), + annotations: vec![], }) } } else { Err(TypeError { message: format!("Cannot call super method because class {} has no parent", type_), span: method.span.clone(), + annotations: vec![], }) } } else { @@ -5510,6 +5540,7 @@ pub fn resolve_super_method(method: &Symbol, env: &SymbolEnv, types: &Types) -> }) .to_string(), span: method.span.clone(), + annotations: vec![], }) } } diff --git a/libs/wingc/src/type_check/symbol_env.rs b/libs/wingc/src/type_check/symbol_env.rs index 513e7dfb07e..c450e91b978 100644 --- a/libs/wingc/src/type_check/symbol_env.rs +++ b/libs/wingc/src/type_check/symbol_env.rs @@ -4,7 +4,7 @@ use duplicate::duplicate_item; use crate::{ ast::{Phase, Symbol}, - diagnostic::TypeError, + diagnostic::{DiagnosticAnnotation, TypeError, WingSpan}, type_check::{SymbolKind, Type, TypeRef}, }; use std::fmt::Debug; @@ -19,7 +19,7 @@ pub type SymbolEnvRef = UnsafeRef; pub struct SymbolEnv { // We use a BTreeMaps here so that we can iterate over the symbols in a deterministic order (snapshot tests) - pub(crate) symbol_map: BTreeMap, + pub(crate) symbol_map: BTreeMap, pub(crate) parent: Option, pub kind: SymbolEnvKind, @@ -41,7 +41,7 @@ impl Display for SymbolEnv { loop { write!(f, "level {}: {{ ", level.to_string().bold())?; let mut items = vec![]; - for (name, (_, kind)) in &env.symbol_map { + for (name, (_, _, kind)) in &env.symbol_map { let repr = match kind { SymbolKind::Type(t) => format!("{} [type]", t).red(), SymbolKind::Variable(v) => format!("{}", v.type_).blue(), @@ -99,7 +99,7 @@ pub enum LookupResult<'a> { MultipleFound, /// The symbol exists in the environment but it's not defined yet (based on the statement /// index passed to the lookup) - DefinedLater, + DefinedLater(WingSpan), /// Expected a namespace in a nested lookup but found a different kind of symbol ExpectedNamespace(Symbol), } @@ -115,7 +115,7 @@ impl<'a> LookupResult<'a> { LookupResult::Found(kind, info) => (kind, info), LookupResult::NotFound(x) => panic!("LookupResult::unwrap({x}) called on LookupResult::NotFound"), LookupResult::MultipleFound => panic!("LookupResult::unwrap() called on LookupResult::MultipleFound"), - LookupResult::DefinedLater => panic!("LookupResult::unwrap() called on LookupResult::DefinedLater"), + LookupResult::DefinedLater(_) => panic!("LookupResult::unwrap() called on LookupResult::DefinedLater"), LookupResult::ExpectedNamespace(symbol) => panic!( "LookupResult::unwrap() called on LookupResult::ExpectedNamespace({:?})", symbol @@ -214,10 +214,16 @@ impl SymbolEnv { return Err(TypeError { span: symbol.span.clone(), message: format!("Symbol \"{}\" already defined in this scope", symbol.name), + annotations: vec![DiagnosticAnnotation { + message: "previous definition".to_string(), + span: self.symbol_map[&symbol.name].1.clone(), + }], }); } - self.symbol_map.insert(symbol.name.clone(), (pos, kind)); + self + .symbol_map + .insert(symbol.name.clone(), (pos, symbol.span.clone(), kind)); Ok(()) } @@ -252,7 +258,7 @@ impl SymbolEnv { /// cannot be a nested symbol (e.g. `foo.bar`), use `lookup_nested` for that. /// TODO: perhaps make this private and switch to the nested version in all external calls pub fn lookup_ext(self: reference([Self]), symbol: &Symbol, not_after_stmt_idx: Option) -> LookupResult { - if let Some((definition_idx, kind)) = self.symbol_map.map_get(&symbol.name) { + if let Some((definition_idx, span, kind)) = self.symbol_map.map_get(&symbol.name) { // if found the symbol and it is defined before the statement index (or statement index is // unspecified, which is likely not something we want to support), we found it let lookup_index = not_after_stmt_idx.unwrap_or(usize::MAX); @@ -262,7 +268,7 @@ impl SymbolEnv { }; if lookup_index < definition_idx { - return LookupResult::DefinedLater; + return LookupResult::DefinedLater(span.clone()); } return LookupResult::Found( @@ -374,7 +380,7 @@ impl SymbolEnv { pub struct SymbolEnvIter<'a> { seen_keys: HashSet, curr_env: &'a SymbolEnv, - curr_pos: btree_map::Iter<'a, String, (StatementIdx, SymbolKind)>, + curr_pos: btree_map::Iter<'a, String, (StatementIdx, WingSpan, SymbolKind)>, with_ancestry: bool, } @@ -393,7 +399,7 @@ impl<'a> Iterator for SymbolEnvIter<'a> { type Item = (String, &'a SymbolKind, SymbolLookupInfo); fn next(&mut self) -> Option { - if let Some((name, (_, kind))) = self.curr_pos.next() { + if let Some((name, (_, _, kind))) = self.curr_pos.next() { if self.seen_keys.contains(name) { self.next() } else { @@ -522,7 +528,7 @@ mod tests { // Lookup positionally visible variable using an index before it's defined assert!(matches!( parent_env.lookup_nested_str("parent_high_pos_var", Some(parent_high_pos_var_idx - 1)), - LookupResult::DefinedLater + LookupResult::DefinedLater(_) )); // Lookup a globally visible parent var in the child env with a low statement index @@ -540,7 +546,7 @@ mod tests { // Lookup a positionally visible parent var defined after the child scope in the child env using a low statement index assert!(matches!( child_env.lookup_nested_str("parent_high_pos_var", Some(0)), - LookupResult::DefinedLater + LookupResult::DefinedLater(_) )); // Lookup for a child var in the parent env diff --git a/libs/wingc/src/valid_json_visitor.rs b/libs/wingc/src/valid_json_visitor.rs index e62fda9e3e6..99b8feebd0f 100644 --- a/libs/wingc/src/valid_json_visitor.rs +++ b/libs/wingc/src/valid_json_visitor.rs @@ -45,6 +45,7 @@ impl<'a> Visit<'_> for ValidJsonVisitor<'a> { report_diagnostic(Diagnostic { message: format!("\"{tt}\" is not a legal JSON value"), span: Some(inner.span.clone()), + annotations: vec![], }) } } @@ -55,6 +56,7 @@ impl<'a> Visit<'_> for ValidJsonVisitor<'a> { report_diagnostic(Diagnostic { message: format!("\"{tt}\" is not a legal JSON value"), span: Some(inner.span.clone()), + annotations: vec![], }) } } @@ -66,6 +68,7 @@ impl<'a> Visit<'_> for ValidJsonVisitor<'a> { report_diagnostic(Diagnostic { message: format!("\"{tt}\" is not a legal JSON value"), span: Some(v.span.clone()), + annotations: vec![], }) } } diff --git a/libs/wingc/src/visit_types.rs b/libs/wingc/src/visit_types.rs index 55887739bc8..3ea9109cb8d 100644 --- a/libs/wingc/src/visit_types.rs +++ b/libs/wingc/src/visit_types.rs @@ -142,7 +142,7 @@ where V: VisitType<'a> + ?Sized, { for variable in node.env.symbol_map.values() { - if let SymbolKind::Variable(variable) = &variable.1 { + if let SymbolKind::Variable(variable) = &variable.2 { v.visit_typeref(&variable.type_); } } @@ -153,7 +153,7 @@ where V: VisitType<'a> + ?Sized, { for field in node.env.symbol_map.values() { - if let SymbolKind::Variable(variable) = &field.1 { + if let SymbolKind::Variable(variable) = &field.2 { v.visit_typeref(&variable.type_); } } @@ -164,7 +164,7 @@ where V: VisitType<'a> + ?Sized, { for field in node.env.symbol_map.values() { - if let SymbolKind::Variable(variable) = &field.1 { + if let SymbolKind::Variable(variable) = &field.2 { v.visit_typeref(&variable.type_); } } @@ -192,7 +192,7 @@ where V: VisitTypeMut<'a> + ?Sized, { for variable in node.env.symbol_map.values_mut() { - if let SymbolKind::Variable(ref mut variable) = variable.1 { + if let SymbolKind::Variable(ref mut variable) = variable.2 { v.visit_typeref_mut(&mut variable.type_); } } @@ -203,7 +203,7 @@ where V: VisitTypeMut<'a> + ?Sized, { for field in node.env.symbol_map.values_mut() { - if let SymbolKind::Variable(ref mut variable) = field.1 { + if let SymbolKind::Variable(ref mut variable) = field.2 { v.visit_typeref_mut(&mut variable.type_); } } @@ -214,7 +214,7 @@ where V: VisitTypeMut<'a> + ?Sized, { for field in node.env.symbol_map.values_mut() { - if let SymbolKind::Variable(ref mut variable) = field.1 { + if let SymbolKind::Variable(ref mut variable) = field.2 { v.visit_typeref_mut(&mut variable.type_); } } diff --git a/libs/wingcompiler/src/wingc.ts b/libs/wingcompiler/src/wingc.ts index f5b9c078a38..3d0ca3d9333 100644 --- a/libs/wingcompiler/src/wingc.ts +++ b/libs/wingcompiler/src/wingc.ts @@ -181,7 +181,7 @@ export async function load(options: WingCompilerLoadOptions) { wasi_snapshot_preview1: wasi.wasiImport, env: { // This function is used only by the lsp - send_diagnostic: () => { }, + send_diagnostic: () => {}, }, ...(options.imports ?? {}), } as any; @@ -226,6 +226,20 @@ export interface WingDiagnostic { }; file_id: string; }; + annotations: { + message: string; + span: { + start: { + line: number; + col: number; + }; + end: { + line: number; + col: number; + }; + file_id: string; + }; + }[]; } /** diff --git a/tools/hangar/__snapshots__/invalid.ts.snap b/tools/hangar/__snapshots__/invalid.ts.snap index c8343332e10..11524ac12b6 100644 --- a/tools/hangar/__snapshots__/invalid.ts.snap +++ b/tools/hangar/__snapshots__/invalid.ts.snap @@ -1554,6 +1554,9 @@ exports[`inflight_reassign.test.w 1`] = ` error: Variable is not reassignable --> ../../../examples/tests/invalid/inflight_reassign.test.w:8:3 | +2 | let ylet = 123; + | ---- defined here (try adding \\"var\\" in front) + . 8 | ylet = 456; | ^^^^ Variable is not reassignable @@ -1670,6 +1673,8 @@ error: Expected an interface, instead found type \\"ISomeClass\\" error: Symbol \\"foo\\" already defined in this scope --> ../../../examples/tests/invalid/interface.test.w:23:5 | +22 | foo(): void; + | --- previous definition 23 | foo(): void; | ^^^ Symbol \\"foo\\" already defined in this scope @@ -1677,6 +1682,9 @@ error: Symbol \\"foo\\" already defined in this scope error: Symbol \\"foo\\" already defined in this scope --> ../../../examples/tests/invalid/interface.test.w:25:5 | +22 | foo(): void; + | --- previous definition + . 25 | foo(): num; | ^^^ Symbol \\"foo\\" already defined in this scope @@ -2154,6 +2162,9 @@ error: Cannot call an optional function error: Variable is not reassignable --> ../../../examples/tests/invalid/optionals.test.w:53:3 | +50 | if let hi = hi { + | -- defined here (try adding \\"var\\" in front) + . 53 | hi = \\"bye\\"; | ^^ Variable is not reassignable @@ -2237,6 +2248,8 @@ exports[`reassign_to_nonreassignable.test.w 1`] = ` "error: Variable is not reassignable --> ../../../examples/tests/invalid/reassign_to_nonreassignable.test.w:3:1 | +2 | let x = 5; + | - defined here (try adding \\"var\\" in front) 3 | x = x + 1; | ^ Variable is not reassignable @@ -2244,6 +2257,8 @@ exports[`reassign_to_nonreassignable.test.w 1`] = ` error: Variable is not reassignable --> ../../../examples/tests/invalid/reassign_to_nonreassignable.test.w:42:3 | +41 | let f = (arg: num):num => { + | --- defined here (try adding \\"var\\" in front) 42 | arg = 0; | ^^^ Variable is not reassignable @@ -2251,6 +2266,9 @@ error: Variable is not reassignable error: Variable is not reassignable --> ../../../examples/tests/invalid/reassign_to_nonreassignable.test.w:35:5 | +16 | inflight inflightF: num; + | --------- defined here (try adding \\"var\\" in front) + . 35 | this.inflightF = this.inflightF + 1; | ^^^^^^^^^^^^^^ Variable is not reassignable @@ -2258,6 +2276,9 @@ error: Variable is not reassignable error: Variable is not reassignable --> ../../../examples/tests/invalid/reassign_to_nonreassignable.test.w:28:5 | +14 | f: num; + | - defined here (try adding \\"var\\" in front) + . 28 | this.f = this.f + 1; | ^^^^^^ Variable is not reassignable @@ -2265,6 +2286,9 @@ error: Variable is not reassignable error: Variable is not reassignable --> ../../../examples/tests/invalid/reassign_to_nonreassignable.test.w:30:5 | + 6 | pub inflight inner: num; + | ----- defined here (try adding \\"var\\" in front) + . 30 | this.innerR.inner = 2; | ^^^^^^^^^^^^^^^^^ Variable is not reassignable @@ -2992,6 +3016,9 @@ error: Symbol \\"x\\" used before being defined | 5 | log(\\"\${x}\\"); | ^ Symbol \\"x\\" used before being defined + . +8 | let x = \\"hi\\"; + | - defined later here