Skip to content

Commit

Permalink
fix(compiler): type errors are resolved as any type by the type che…
Browse files Browse the repository at this point in the history
…cker (#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)*.
  • Loading branch information
yoav-steinberg committed Jun 21, 2023
1 parent 1026d18 commit 7513700
Show file tree
Hide file tree
Showing 12 changed files with 263 additions and 150 deletions.
3 changes: 3 additions & 0 deletions examples/tests/invalid/unknown_symbol.w
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,6 @@ class SomeResource {
class A extends B {
//^ Unknown symbol
}

unknown = 1;
//^ Unknown symbol
1 change: 1 addition & 0 deletions libs/wingc/src/docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ impl Documented for TypeRef {
| Type::Json
| Type::MutJson
| Type::Nil
| Type::Unresolved
| Type::Array(_)
| Type::MutArray(_)
| Type::Map(_)
Expand Down
1 change: 1 addition & 0 deletions libs/wingc/src/jsify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
}
}
}
Expand Down
20 changes: 15 additions & 5 deletions libs/wingc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<JsiiImportSpec>,
) {
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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down
34 changes: 26 additions & 8 deletions libs/wingc/src/lsp/completions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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::<Vec<_>>();
// 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));
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 13 additions & 1 deletion libs/wingc/src/lsp/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -25,6 +26,9 @@ pub struct FileData {
pub scope: Box<Scope>,
/// 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<JsiiImportSpec>,
}

thread_local! {
Expand Down Expand Up @@ -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 --

Expand All @@ -133,6 +144,7 @@ fn partial_compile(source_file: &str, text: &[u8], jsii_types: &mut TypeSystem)
diagnostics: get_diagnostics(),
scope,
types,
jsii_imports,
};
}

Expand Down
Loading

0 comments on commit 7513700

Please sign in to comment.