From 7513700325ce752758afaa8f1a0eff9e9ad766a9 Mon Sep 17 00:00:00 2001 From: yoav-steinberg Date: Wed, 21 Jun 2023 21:29:43 +0300 Subject: [PATCH] fix(compiler): type errors are resolved as `any` type by the type checker (#2924) See #884 Since type errors resolved as `any` type by the type checker we don't have a cleat way to distinguish unresolved types from valid `any` types. This can lead to bug downstream where a symbol with an unknown type is accessed as if it's an `any` without the compiler noticing it. In practice this usually isn't a problem since type errors lead to a diagnostic which leads to a compilation failure so we never get too far downstream with this. But correctness is important here: * There might be cases we aren't aware of. * A dedicated error-type can be used for additional diagnostics like location where the type resolving failed. * We are now more explicit in the code about ignore cascading errors as opposed to handing an any type. - [x] ~~As of now I don't have a good test for this showing how using an `any` type creates an error where it's illegal compared to ignoring that error in case of a cascading type error.~~ But I'm working on finding such a case... **update:** The one case that's relevant that I could find is that unknown symbols are now "unresolved types" which means we don't need to check if they are reassignable in assignment statements, previously they were any types which lead to an additional redundant error (can't reassign to a non-reassignable). Updated snapshot test accordingly. The PR also cleans up some type checking code and produces slightly better diags in some cases while eliminating diags which where redundant (see snapshot). This PR also changes the LSP to eagerly load submodule types when needed in the completion logic instead of during the type checking phase. This is a revised fix of #2921. ## Checklist - [x] Title matches [Winglang's style guide](https://docs.winglang.io/contributors/pull_requests#how-are-pull-request-titles-formatted) - [x] Description explains motivation and solution - [ ] 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 [Monada Contribution License](https://docs.winglang.io/terms-and-policies/contribution-license.html)*. --- examples/tests/invalid/unknown_symbol.w | 3 + libs/wingc/src/docs.rs | 1 + libs/wingc/src/jsify.rs | 1 + libs/wingc/src/lib.rs | 20 +- libs/wingc/src/lsp/completions.rs | 34 ++- .../only_show_symbols_in_scope.snap | 2 +- libs/wingc/src/lsp/sync.rs | 14 +- libs/wingc/src/type_check.rs | 250 ++++++++++-------- libs/wingc/src/type_check/jsii_importer.rs | 39 ++- libs/wingc/src/type_check/symbol_env.rs | 2 + libs/wingc/src/type_check_assert.rs | 22 +- tools/hangar/__snapshots__/invalid.ts.snap | 25 +- 12 files changed, 263 insertions(+), 150 deletions(-) diff --git a/examples/tests/invalid/unknown_symbol.w b/examples/tests/invalid/unknown_symbol.w index 7a67445d3a6..00026e2c0fe 100644 --- a/examples/tests/invalid/unknown_symbol.w +++ b/examples/tests/invalid/unknown_symbol.w @@ -28,3 +28,6 @@ class SomeResource { class A extends B { //^ Unknown symbol } + +unknown = 1; +//^ Unknown symbol diff --git a/libs/wingc/src/docs.rs b/libs/wingc/src/docs.rs index cf94bdbc53f..3a979dff8cf 100644 --- a/libs/wingc/src/docs.rs +++ b/libs/wingc/src/docs.rs @@ -77,6 +77,7 @@ impl Documented for TypeRef { | Type::Json | Type::MutJson | Type::Nil + | Type::Unresolved | Type::Array(_) | Type::MutArray(_) | Type::Map(_) diff --git a/libs/wingc/src/jsify.rs b/libs/wingc/src/jsify.rs index 60759a5dabf..65755cff968 100644 --- a/libs/wingc/src/jsify.rs +++ b/libs/wingc/src/jsify.rs @@ -1787,6 +1787,7 @@ impl<'a> FieldReferenceVisitor<'a> { Type::Class(cls) => Some(ComponentKind::Member(cls.env.lookup(&property, None)?.as_variable()?)), Type::Interface(iface) => Some(ComponentKind::Member(iface.env.lookup(&property, None)?.as_variable()?)), Type::Struct(st) => Some(ComponentKind::Member(st.env.lookup(&property, None)?.as_variable()?)), + Type::Unresolved => panic!("Encountered unresolved type during jsification"), } } } diff --git a/libs/wingc/src/lib.rs b/libs/wingc/src/lib.rs index 02e2eb967ae..f0d582c74d8 100644 --- a/libs/wingc/src/lib.rs +++ b/libs/wingc/src/lib.rs @@ -13,6 +13,7 @@ use comp_ctx::set_custom_panic_hook; use diagnostic::{found_errors, report_diagnostic, Diagnostic}; use fold::Fold; use jsify::JSifier; +use type_check::jsii_importer::JsiiImportSpec; use type_check::symbol_env::StatementIdx; use type_check::{FunctionSignature, SymbolKind, Type}; use type_check_assert::TypeCheckAssert; @@ -168,7 +169,13 @@ pub fn parse(source_path: &Path) -> Scope { wing_parser.wingit(&tree.root_node()) } -pub fn type_check(scope: &mut Scope, types: &mut Types, source_path: &Path, jsii_types: &mut TypeSystem) { +pub fn type_check( + scope: &mut Scope, + types: &mut Types, + source_path: &Path, + jsii_types: &mut TypeSystem, + jsii_imports: &mut Vec, +) { assert!(scope.env.borrow().is_none(), "Scope should not have an env yet"); let env = SymbolEnv::new(None, types.void(), false, Phase::Preflight, 0); scope.set_env(env); @@ -244,7 +251,7 @@ pub fn type_check(scope: &mut Scope, types: &mut Types, source_path: &Path, jsii types, ); - let mut tc = TypeChecker::new(types, source_path, jsii_types); + let mut tc = TypeChecker::new(types, source_path, jsii_types, jsii_imports); tc.add_globals(scope); tc.type_check_scope(scope); @@ -309,11 +316,14 @@ pub fn compile( let mut types = Types::new(); let mut jsii_types = TypeSystem::new(); + // Create a universal JSII import spec (need to keep this alive during entire compilation) + let mut jsii_imports = vec![]; + // Type check everything and build typed symbol environment - type_check(&mut scope, &mut types, &source_path, &mut jsii_types); + type_check(&mut scope, &mut types, &source_path, &mut jsii_types, &mut jsii_imports); - // Validate that every Expr in the final tree has been type checked - let mut tc_assert = TypeCheckAssert::new(&types); + // Validate the type checker didn't miss anything see `TypeCheckAssert` for details + let mut tc_assert = TypeCheckAssert::new(&types, found_errors()); tc_assert.check(&scope); // bail out now (before jsification) if there are errors (no point in jsifying) diff --git a/libs/wingc/src/lsp/completions.rs b/libs/wingc/src/lsp/completions.rs index 6b92987d605..1dd3c08beba 100644 --- a/libs/wingc/src/lsp/completions.rs +++ b/libs/wingc/src/lsp/completions.rs @@ -5,20 +5,22 @@ use lsp_types::{ use std::cmp::max; use tree_sitter::Point; -use crate::ast::{Expr, ExprKind, Phase, Scope, TypeAnnotation, TypeAnnotationKind}; +use crate::ast::{Expr, ExprKind, Phase, Scope, Symbol, TypeAnnotation, TypeAnnotationKind, UserDefinedType}; use crate::closure_transform::{CLOSURE_CLASS_PREFIX, PARENT_THIS_NAME}; use crate::diagnostic::{WingLocation, WingSpan}; use crate::docs::Documented; use crate::lsp::sync::FILES; use crate::type_check::symbol_env::{LookupResult, StatementIdx}; use crate::type_check::{ - resolve_user_defined_type, ClassLike, Namespace, SymbolKind, Type, Types, UnsafeRef, CLASS_INFLIGHT_INIT_NAME, - CLASS_INIT_NAME, + import_udt_from_jsii, resolve_user_defined_type, ClassLike, Namespace, SymbolKind, Type, Types, UnsafeRef, + CLASS_INFLIGHT_INIT_NAME, CLASS_INIT_NAME, }; use crate::visit::{visit_expr, visit_type_annotation, Visit}; use crate::wasm_util::{ptr_to_string, string_to_combined_ptr, WASM_RETURN_ERROR}; use crate::{WINGSDK_ASSEMBLY_NAME, WINGSDK_STD_MODULE}; +use super::sync::JSII_TYPES; + #[no_mangle] pub unsafe extern "C" fn wingc_on_completion(ptr: u32, len: u32) -> u64 { let parse_string = ptr_to_string(ptr, len); @@ -35,9 +37,9 @@ pub unsafe extern "C" fn wingc_on_completion(ptr: u32, len: u32) -> u64 { pub fn on_completion(params: lsp_types::CompletionParams) -> CompletionResponse { let mut final_completions = FILES.with(|files| { - let files = files.borrow(); + let mut files = files.borrow_mut(); let uri = params.text_document_position.text_document.uri; - let file_data = files.get(&uri).expect("File must be open to get completions"); + let file_data = files.get_mut(&uri).expect("File must be open to get completions"); let types = &file_data.types; let root_scope = &file_data.scope; @@ -91,8 +93,8 @@ pub fn on_completion(params: lsp_types::CompletionParams) -> CompletionResponse if let Some(nearest_expr) = scope_visitor.nearest_expr { let nearest_expr_type = types.get_expr_type(nearest_expr).unwrap(); - // If we are inside an incomplete reference, there is possibly a type error so we can't trust "any" - if !nearest_expr_type.is_anything() { + // If we are inside an incomplete reference, there is possibly a type error or an anything which has no completions + if !nearest_expr_type.is_anything() && !nearest_expr_type.is_unresolved() { return get_completions_from_type( &nearest_expr_type, types, @@ -136,6 +138,21 @@ pub fn on_completion(params: lsp_types::CompletionParams) -> CompletionResponse ) } SymbolKind::Namespace(n) => { + // If the types in this namespace aren't loaded yet, load them now to get completions + if !n.loaded { + JSII_TYPES.with(|jsii_types| { + let mut jsii_types = jsii_types.borrow_mut(); + let parts = reference_text.split(".").collect::>(); + // Dummy type representing the namespace to be loaded + let udt = UserDefinedType { + root: Symbol::global(parts[0].to_string()), + fields: parts[1..].iter().map(|s| Symbol::global(s.to_string())).collect(), + span: WingSpan::default(), + }; + // Import all types in the namespace by trying to load the "dummy type" + import_udt_from_jsii(&mut file_data.types, &mut jsii_types, &udt, &file_data.jsii_imports); + }); + } return get_completions_from_namespace(&n, Some(found_env.phase)); } } @@ -307,7 +324,7 @@ fn get_completions_from_type( .collect() } Type::Optional(t) => get_completions_from_type(t, types, current_phase, is_instance), - Type::Void | Type::Function(_) | Type::Anything => vec![], + Type::Void | Type::Function(_) | Type::Anything | Type::Unresolved => vec![], Type::Number | Type::String | Type::Duration @@ -505,6 +522,7 @@ fn format_symbol_kind_as_completion(name: &str, symbol_kind: &SymbolKind) -> Opt | Type::Json | Type::MutJson | Type::Nil + | Type::Unresolved | Type::Optional(_) => CompletionItemKind::CONSTANT, Type::Function(_) => CompletionItemKind::FUNCTION, Type::Struct(_) => CompletionItemKind::STRUCT, diff --git a/libs/wingc/src/lsp/snapshots/completions/only_show_symbols_in_scope.snap b/libs/wingc/src/lsp/snapshots/completions/only_show_symbols_in_scope.snap index c3e4b1685c0..dff5f7f4477 100644 --- a/libs/wingc/src/lsp/snapshots/completions/only_show_symbols_in_scope.snap +++ b/libs/wingc/src/lsp/snapshots/completions/only_show_symbols_in_scope.snap @@ -24,7 +24,7 @@ source: libs/wingc/src/lsp/completions.rs kind: 6 documentation: kind: markdown - value: "```wing\npreflight b: any\n```\n---" + value: "```wing\npreflight b: unresolved\n```\n---" sortText: bb|b insertText: b insertTextFormat: 2 diff --git a/libs/wingc/src/lsp/sync.rs b/libs/wingc/src/lsp/sync.rs index 3820a77929e..695ddf6973d 100644 --- a/libs/wingc/src/lsp/sync.rs +++ b/libs/wingc/src/lsp/sync.rs @@ -11,6 +11,7 @@ use crate::fold::Fold; use crate::jsify::JSifier; use crate::parser::Parser; use crate::type_check; +use crate::type_check::jsii_importer::JsiiImportSpec; use crate::{ast::Scope, type_check::Types, wasm_util::ptr_to_string}; /// The result of running wingc on a file @@ -25,6 +26,9 @@ pub struct FileData { pub scope: Box, /// The universal type collection for the scope. This is saved to ensure references live long enough. pub types: Types, + /// The JSII imports for the file. This is saved so we can load JSII types (for autotocompletion for example) + /// which don't exist explicitly in the source. + pub jsii_imports: Vec, } thread_local! { @@ -114,8 +118,15 @@ fn partial_compile(source_file: &str, text: &[u8], jsii_types: &mut TypeSystem) let mut scope = Box::new(inflight_transformer.fold_scope(scope)); // -- TYPECHECKING PHASE -- + let mut jsii_imports = vec![]; - type_check(&mut scope, &mut types, &Path::new(source_file), jsii_types); + type_check( + &mut scope, + &mut types, + &Path::new(source_file), + jsii_types, + &mut jsii_imports, + ); // -- JSIFICATION PHASE -- @@ -133,6 +144,7 @@ fn partial_compile(source_file: &str, text: &[u8], jsii_types: &mut TypeSystem) diagnostics: get_diagnostics(), scope, types, + jsii_imports, }; } diff --git a/libs/wingc/src/type_check.rs b/libs/wingc/src/type_check.rs index 4f3ba274180..1b19bf639c9 100644 --- a/libs/wingc/src/type_check.rs +++ b/libs/wingc/src/type_check.rs @@ -160,6 +160,7 @@ pub enum Type { Json, MutJson, Nil, + Unresolved, Optional(TypeRef), Array(TypeRef), MutArray(TypeRef), @@ -186,6 +187,12 @@ pub struct Namespace { #[derivative(Debug = "ignore")] pub env: SymbolEnv, + + // Indicate whether all the types in this namespace have been loaded, this is part of our + // lazy loading mechanism and is used by the lsp's autocomplete in case we need to load + // the types after initial compilation. + #[derivative(Debug = "ignore")] + pub loaded: bool, } pub type NamespaceRef = UnsafeRef; @@ -646,14 +653,6 @@ impl FunctionSignature { self.parameters.len() - num_optionals } - - /// Returns the maximum number of parameters that can be passed to this function. - /// - /// TODO: how to represent unlimited parameters in the case of variadics? - /// https://github.com/winglang/wing/issues/125 - fn max_parameters(&self) -> usize { - self.parameters.len() - } } impl PartialEq for FunctionSignature { @@ -690,6 +689,7 @@ impl Display for Type { Type::Json => write!(f, "Json"), Type::MutJson => write!(f, "MutJson"), Type::Nil => write!(f, "nil"), + Type::Unresolved => write!(f, "unresolved"), Type::Optional(v) => write!(f, "{}?", v), Type::Function(sig) => write!(f, "{}", sig), Type::Class(class) => write!(f, "{}", class.name.name), @@ -806,6 +806,10 @@ impl TypeRef { matches!(**self, Type::Anything) } + pub fn is_unresolved(&self) -> bool { + matches!(**self, Type::Unresolved) + } + pub fn is_preflight_class(&self) -> bool { if let Type::Class(ref class) = **self { return class.phase == Phase::Preflight; @@ -888,6 +892,7 @@ impl TypeRef { Type::Struct(_) => true, Type::Optional(v) => v.is_capturable(), Type::Anything => false, + Type::Unresolved => false, Type::Void => false, Type::MutJson => false, Type::MutArray(_) => false, @@ -977,6 +982,7 @@ pub struct Types { json_idx: usize, mut_json_idx: usize, nil_idx: usize, + err_idx: usize, type_for_expr: Vec>, @@ -1004,6 +1010,8 @@ impl Types { let mut_json_idx = types.len() - 1; types.push(Box::new(Type::Nil)); let nil_idx = types.len() - 1; + types.push(Box::new(Type::Unresolved)); + let err_idx = types.len() - 1; // TODO: this is hack to create the top-level mapping from lib names to symbols // We construct a void ref by hand since we can't call self.void() while constructing the Types struct @@ -1023,6 +1031,7 @@ impl Types { json_idx, mut_json_idx, nil_idx, + err_idx, type_for_expr: Vec::new(), resource_base_type: None, } @@ -1053,7 +1062,7 @@ impl Types { } pub fn error(&self) -> TypeRef { - self.get_typeref(self.anything_idx) + self.get_typeref(self.err_idx) } pub fn void(&self) -> TypeRef { @@ -1152,7 +1161,7 @@ pub struct TypeChecker<'a> { /// JSII Manifest descriptions to be imported. /// May be reused between compilations - jsii_imports: Vec, + jsii_imports: &'a mut Vec, /// The JSII type system jsii_types: &'a mut TypeSystem, @@ -1165,13 +1174,18 @@ pub struct TypeChecker<'a> { } impl<'a> TypeChecker<'a> { - pub fn new(types: &'a mut Types, source_path: &'a Path, jsii_types: &'a mut TypeSystem) -> Self { + pub fn new( + types: &'a mut Types, + source_path: &'a Path, + jsii_types: &'a mut TypeSystem, + jsii_imports: &'a mut Vec, + ) -> Self { Self { types, inner_scopes: vec![], jsii_types, source_path, - jsii_imports: vec![], + jsii_imports, in_json: 0, statement_idx: 0, } @@ -1268,13 +1282,16 @@ impl<'a> TypeChecker<'a> { } else if ltype.is_subtype_of(&self.types.string()) && rtype.is_subtype_of(&self.types.string()) { self.types.string() } else { - self.spanned_error( - exp, - format!( - "Binary operator '+' cannot be applied to operands of type '{}' and '{}'; only ({}, {}) and ({}, {}) are supported", - ltype, rtype, self.types.number(), self.types.number(), self.types.string(), self.types.string(), - ), - ); + // If any of the types are unresolved (error) then don't report this assuming the error has already been reported + if !ltype.is_unresolved() && !rtype.is_unresolved() { + self.spanned_error( + exp, + format!( + "Binary operator '+' cannot be applied to operands of type '{}' and '{}'; only ({}, {}) and ({}, {}) are supported", + ltype, rtype, self.types.number(), self.types.number(), self.types.string(), self.types.string(), + ), + ); + } self.types.error() } } @@ -1369,22 +1386,23 @@ impl<'a> TypeChecker<'a> { return self.types.error(); } } - t => { - if matches!(t, Type::Anything) { - return self.types.anything(); - } else if matches!(t, Type::Struct(_)) { - self.spanned_error( - class, - format!("Cannot instantiate type \"{}\" because it is a struct and not a class. Use struct instantiation instead.", type_), - ); - return self.types.error(); - } else { - self.spanned_error( - class, - format!("Cannot instantiate type \"{}\" because it is not a class", type_), - ); - return self.types.error(); - } + // If type is anything we have to assume it's ok to initialize it + Type::Anything => return self.types.anything(), + // If type is error, we assume the error was already reported and evauate the new expression to error as well + Type::Unresolved => return self.types.error(), + Type::Struct(_) => { + self.spanned_error( + class, + format!("Cannot instantiate type \"{}\" because it is a struct and not a class. Use struct instantiation instead.", type_), + ); + return self.types.error(); + } + _ => { + self.spanned_error( + class, + format!("Cannot instantiate type \"{}\" because it is not a class", type_), + ); + return self.types.error(); } }; @@ -1452,8 +1470,13 @@ impl<'a> TypeChecker<'a> { let arg_list_types = self.type_check_arg_list(arg_list, env); - // TODO: hack to support methods of stdlib object we don't know their types yet (basically stuff like cloud.Bucket().upload()) - if matches!(*func_type, Type::Anything) { + // If the callee's signature type is unknown, just evaluate the entire call expression as an error + if func_type.is_unresolved() { + return self.types.error(); + } + + // If the caller's signature is `any`, then just evaluate the entire call expression as `any` + if func_type.is_anything() { return self.types.anything(); } @@ -1476,7 +1499,10 @@ impl<'a> TypeChecker<'a> { return self.types.error(); } } else { - self.spanned_error(callee, "Expected a function or method"); + self.spanned_error( + callee, + format!("Expected a function or method, found \"{}\"", func_type), + ); return self.types.error(); }; @@ -1866,41 +1892,52 @@ impl<'a> TypeChecker<'a> { /// Returns the given type on success, otherwise returns one of the expected types. fn validate_type_in(&mut self, actual_type: TypeRef, expected_types: &[TypeRef], span: &impl Spanned) -> TypeRef { assert!(expected_types.len() > 0); - if !actual_type.is_anything() - && !expected_types + + // If the actual type is anything or any of the expected types then we're good + if actual_type.is_anything() || expected_types.iter().any(|t| actual_type.is_subtype_of(t)) { + return actual_type; + } + + // If the actual type is an error (a type we failed to resolve) then we silently ignore it assuming + // the error was already reported. + if actual_type.is_unresolved() { + return actual_type; + } + + // If any of the expected types are errors (types we failed to resolve) then we silently ignore it + // assuming the error was already reported. + if expected_types.iter().any(|t| t.is_unresolved()) { + return actual_type; + } + + let expected_type_str = if expected_types.len() > 1 { + let expected_types_list = expected_types .iter() - .any(|expected| actual_type.is_subtype_of(&expected)) - { - report_diagnostic(Diagnostic { - message: if expected_types.len() > 1 { - let expected_types_list = expected_types - .iter() - .map(|t| format!("{}", t)) - .collect::>() - .join(","); - format!( - "Expected type to be one of \"{}\", but got \"{}\" instead", - expected_types_list, actual_type - ) - } else { - let mut message = format!( - "Expected type to be \"{}\", but got \"{}\" instead", - expected_types[0], actual_type - ); - if actual_type.is_nil() { - message = format!( - "{} (hint: to allow \"nil\" assignment use optional type: \"{}?\")", - message, expected_types[0] - ); - } - message - }, - span: Some(span.span()), - }); - expected_types[0] + .map(|t| format!("{}", t)) + .collect::>() + .join(","); + format!("one of \"{}\"", expected_types_list) } else { - actual_type + format!("\"{}\"", expected_types[0]) + }; + + let mut message = format!( + "Expected type to be {}, but got \"{}\" instead", + expected_type_str, actual_type + ); + if actual_type.is_nil() && expected_types.len() == 1 { + message = format!( + "{} (hint: to allow \"nil\" assignment use optional type: \"{}?\")", + message, expected_types[0] + ); } + report_diagnostic(Diagnostic { + message, + span: Some(span.span()), + }); + + // Evaluate to one of the expected types + expected_types[0] } pub fn type_check_scope(&mut self, scope: &Scope) { @@ -2212,7 +2249,7 @@ impl<'a> TypeChecker<'a> { StmtKind::Assignment { variable, value } => { let exp_type = self.type_check_exp(value, env); let var_info = self.resolve_reference(variable, env); - if !var_info.reassignable { + if !var_info.type_.is_unresolved() && !var_info.reassignable { self.spanned_error(stmt, format!("Variable {} is not reassignable ", variable)); } self.validate_type(exp_type, var_info.type_, value); @@ -2521,7 +2558,7 @@ impl<'a> TypeChecker<'a> { Some(t) } else { // The type checker resolves non-existing definitions to `any`, so we avoid duplicate errors by checking for that here - if !t.is_anything() { + if !t.is_unresolved() { self.spanned_error(i, format!("Expected an interface, instead found type \"{}\"", t)); } None @@ -2983,7 +3020,11 @@ impl<'a> TypeChecker<'a> { // If we're importing from the the wing sdk, eagerly import all the types within it // The wing sdk is special because it's currently the only jsii module we import with a specific target namespace if jsii.assembly_name == WINGSDK_ASSEMBLY_NAME { - importer.deep_import_submodule_to_env(&jsii.alias.name); + importer.deep_import_submodule_to_env(if jsii.namespace_filter.is_empty() { + None + } else { + Some(jsii.namespace_filter.join(".")) + }); } importer.import_root_types(); @@ -3337,11 +3378,10 @@ impl<'a> TypeChecker<'a> { // Give a specific error message if someone tries to write "print" instead of "log" if symbol.name == "print" { self.spanned_error(symbol, "Unknown symbol \"print\", did you mean to use \"log\"?"); - self.make_error_variable_info(false) } else { self.type_error(lookup_result_to_type_error(lookup_res, symbol)); - self.make_error_variable_info(false) } + self.make_error_variable_info(false) } } Reference::InstanceMember { @@ -3387,18 +3427,9 @@ impl<'a> TypeChecker<'a> { } let instance_type = self.type_check_exp(object, env); - - // TODO Use error type instead of "anything" here https://github.com/winglang/wing/issues/884 - if instance_type.is_anything() { - // Check to see if this reference is actually an invalid usage of a namespace - if let Some(ref_udt) = self.reference_to_udt(reference) { - let lookup = self.resolve_user_defined_type(&ref_udt, env, self.statement_idx); - if let Err(t) = lookup { - if t.message.ends_with("to be a type but it's a namespace") { - return self.make_error_variable_info(false); - } - } - } + // If resolving the object's type failed, we can't resolve the property either + if instance_type.is_unresolved() { + return self.make_error_variable_info(false); } let res = self.resolve_variable_from_instance_type(instance_type, property, env, object); @@ -3616,25 +3647,8 @@ impl<'a> TypeChecker<'a> { } // If the type is not found, attempt to import it from a jsii library - for jsii in &*self.jsii_imports { - if jsii.alias.name == user_defined_type.root.name { - let mut importer = JsiiImporter::new(&jsii, self.types, self.jsii_types); - - let mut udt_string = if jsii.assembly_name == WINGSDK_ASSEMBLY_NAME { - // when importing from the std lib, the "alias" is the submodule - format!("{}.{}.", jsii.assembly_name, jsii.alias.name) - } else { - format!("{}.", jsii.assembly_name) - }; - udt_string.push_str(&user_defined_type.fields.iter().map(|g| g.name.clone()).join(".")); - - if importer.import_type(&FQN::from(udt_string.as_str())) { - return resolve_user_defined_type(user_defined_type, env, statement_idx); - } else { - // if the import failed, don't bother trying to do any more lookups - break; - } - } + if import_udt_from_jsii(self.types, self.jsii_types, user_defined_type, &self.jsii_imports) { + return resolve_user_defined_type(user_defined_type, env, statement_idx); } // If the type is still not found, return the original error @@ -3853,6 +3867,30 @@ pub fn resolve_user_defined_type( } } +pub fn import_udt_from_jsii( + wing_types: &mut Types, + jsii_types: &mut TypeSystem, + user_defined_type: &UserDefinedType, + jsii_imports: &[JsiiImportSpec], +) -> bool { + for jsii in jsii_imports { + if jsii.alias.name == user_defined_type.root.name { + let mut importer = JsiiImporter::new(&jsii, wing_types, jsii_types); + + let mut udt_string = if jsii.assembly_name == WINGSDK_ASSEMBLY_NAME { + // when importing from the std lib, the "alias" is the submodule + format!("{}.{}.", jsii.assembly_name, jsii.alias.name) + } else { + format!("{}.", jsii.assembly_name) + }; + udt_string.push_str(&user_defined_type.fields.iter().map(|g| g.name.clone()).join(".")); + + return importer.import_type(&FQN::from(udt_string.as_str())); + } + } + false +} + #[cfg(test)] mod tests { use super::*; diff --git a/libs/wingc/src/type_check/jsii_importer.rs b/libs/wingc/src/type_check/jsii_importer.rs index e6f9d48ae87..72b42e7e76e 100644 --- a/libs/wingc/src/type_check/jsii_importer.rs +++ b/libs/wingc/src/type_check/jsii_importer.rs @@ -151,7 +151,7 @@ impl<'a> JsiiImporter<'a> { if let LookupResult::Found(sym, ..) = self.wing_types.libraries.lookup_nested_str(type_str, None) { if let SymbolKind::Namespace(n) = sym { // We are trying to import a namespace directly, so let's eagerly load all of its types - self.deep_import_submodule_to_env(n.name.clone().as_str()); + self.deep_import_submodule_to_env(Some(n.name.clone())); } return true; } @@ -201,6 +201,7 @@ impl<'a> JsiiImporter<'a> { let ns = self.wing_types.add_namespace(Namespace { name: type_name.assembly().to_string(), env: SymbolEnv::new(None, self.wing_types.void(), false, Phase::Preflight, 0), + loaded: false, }); self .wing_types @@ -242,6 +243,7 @@ impl<'a> JsiiImporter<'a> { let ns = self.wing_types.add_namespace(Namespace { name: namespace_name.to_string(), env: SymbolEnv::new(None, self.wing_types.void(), false, Phase::Preflight, 0), + loaded: false, }); parent_ns .env @@ -775,12 +777,11 @@ impl<'a> JsiiImporter<'a> { } /// Imports all types within a given submodule - pub fn deep_import_submodule_to_env(&mut self, submodule: &str) { - let sub_string = Some(submodule.to_string()); + pub fn deep_import_submodule_to_env(&mut self, submodule: Option) { let match_namespace = |jsii_type: &jsii::Type| match jsii_type { - jsii::Type::ClassType(c) => c.namespace == sub_string, - jsii::Type::EnumType(e) => e.namespace == sub_string, - jsii::Type::InterfaceType(i) => i.namespace == sub_string, + jsii::Type::ClassType(c) => c.namespace == submodule, + jsii::Type::EnumType(e) => e.namespace == submodule, + jsii::Type::InterfaceType(i) => i.namespace == submodule, }; for entry in self @@ -800,6 +801,27 @@ impl<'a> JsiiImporter<'a> { break; } } + + // Mark the namespace as loaded after recursively importing all types to its environment + let mut submodule_fqn = self.jsii_spec.assembly_name.clone(); + if let Some(submodule) = submodule { + submodule_fqn.push_str(&format!(".{}", submodule)); + } + self.mark_namespace_as_loaded(&submodule_fqn); + } + + fn mark_namespace_as_loaded(&mut self, module_name: &str) { + let n = self + .wing_types + .libraries + .lookup_nested_str_mut(&module_name, None) + .ok() + .expect(&format!("Namespace {} to be in libraries", module_name)) + .0 + .as_namespace_mut() + .expect(&format!("{} to be a namespace", module_name)); + + n.loaded = true; } /// Import all top-level types that are not in a submodule @@ -856,6 +878,7 @@ impl<'a> JsiiImporter<'a> { let ns = self.wing_types.add_namespace(Namespace { name: assembly.name.clone(), env: SymbolEnv::new(None, self.wing_types.void(), false, Phase::Preflight, 0), + loaded: false, }); self .wing_types @@ -869,6 +892,10 @@ impl<'a> JsiiImporter<'a> { } } + // Mark the namespace as loaded. Note that its inner submodules might not be marked as + // loaded yet. But if they'll be accessed then they'll be marked as well. + self.mark_namespace_as_loaded(&assembly.name); + // Create a symbol in the environment for the imported module // For example, `bring cloud` will create a symbol named `cloud` in the environment // that points to the `@winglang/sdk.cloud` NamespaceRef diff --git a/libs/wingc/src/type_check/symbol_env.rs b/libs/wingc/src/type_check/symbol_env.rs index 3ef903176f2..5916b38f584 100644 --- a/libs/wingc/src/type_check/symbol_env.rs +++ b/libs/wingc/src/type_check/symbol_env.rs @@ -551,10 +551,12 @@ mod tests { let ns1 = types.add_namespace(Namespace { name: "ns1".to_string(), env: SymbolEnv::new(None, types.void(), false, Phase::Independent, 0), + loaded: false, }); let ns2 = types.add_namespace(Namespace { name: "ns2".to_string(), env: SymbolEnv::new(Some(ns1.env.get_ref()), types.void(), false, Phase::Independent, 0), + loaded: false, }); // Define ns2 in n1's env diff --git a/libs/wingc/src/type_check_assert.rs b/libs/wingc/src/type_check_assert.rs index 95b40bce079..1786b9fe374 100644 --- a/libs/wingc/src/type_check_assert.rs +++ b/libs/wingc/src/type_check_assert.rs @@ -4,13 +4,17 @@ use crate::{ visit::{self, Visit}, }; +/// This visitor validates: +/// 1. All expressions in the AST were resolved to some type. +/// 2. If any were resolved to `Unresolved` then we should have generated some error diagnostics. pub struct TypeCheckAssert<'a> { types: &'a Types, + tc_found_errors: bool, } impl<'a> TypeCheckAssert<'a> { - pub fn new(types: &'a Types) -> Self { - Self { types } + pub fn new(types: &'a Types, tc_found_errors: bool) -> Self { + Self { types, tc_found_errors } } pub fn check(&mut self, scope: &Scope) { @@ -20,11 +24,15 @@ impl<'a> TypeCheckAssert<'a> { impl<'a> Visit<'_> for TypeCheckAssert<'a> { fn visit_expr(&mut self, expr: &Expr) { - assert!( - self.types.get_expr_type(expr).is_some(), - "Expr was not type checked: {:?}", - expr, - ); + if let Some(t) = self.types.get_expr_type(expr) { + assert!( + self.tc_found_errors || !t.is_unresolved(), + "Expr's type was not resolved: {:?}", + expr + ); + } else { + panic!("Expr was not type checked: {:?}", expr) + } visit::visit_expr(self, expr); } } diff --git a/tools/hangar/__snapshots__/invalid.ts.snap b/tools/hangar/__snapshots__/invalid.ts.snap index 8fd71b49ee6..96d0c02d8e9 100644 --- a/tools/hangar/__snapshots__/invalid.ts.snap +++ b/tools/hangar/__snapshots__/invalid.ts.snap @@ -47,13 +47,6 @@ error: Unknown symbol \\"this\\" | ^^^^ Unknown symbol \\"this\\" -error: Variable this.instanceField is not reassignable - --> ../../../examples/tests/invalid/access_static_from_instance.w:7:5 - | -7 | this.instanceField = 1; // Can't access instance fields from static methods - | ^^^^^^^^^^^^^^^^^^^^^^^ Variable this.instanceField is not reassignable - - error: Unknown symbol \\"this\\" --> ../../../examples/tests/invalid/access_static_from_instance.w:8:5 | @@ -61,13 +54,6 @@ error: Unknown symbol \\"this\\" | ^^^^ Unknown symbol \\"this\\" -error: Variable this.f is not reassignable - --> ../../../examples/tests/invalid/access_static_from_instance.w:8:5 - | -8 | this.f = 1; // Can't access static fields through \`this\` - | ^^^^^^^^^^^ Variable this.f is not reassignable - - @@ -1499,11 +1485,11 @@ Duration `; exports[`resource_access_field_as_method.w 1`] = ` -"error: Expected a function or method +"error: Expected a function or method, found \\"str\\" --> ../../../examples/tests/invalid/resource_access_field_as_method.w:9:1 | 9 | x.name(); - | ^^^^^^ Expected a function or method + | ^^^^^^ Expected a function or method, found \\"str\\" @@ -2017,6 +2003,13 @@ error: Unknown symbol \\"B\\" | ^ Unknown symbol \\"B\\" +error: Unknown symbol \\"unknown\\" + --> ../../../examples/tests/invalid/unknown_symbol.w:32:1 + | +32 | unknown = 1; + | ^^^^^^^ Unknown symbol \\"unknown\\" + + error: Unknown symbol \\"assert\\" --> ../../../examples/tests/invalid/unknown_symbol.w:20:17 |