Skip to content

Commit

Permalink
fix(compiler)!: unexpected heuristic for inference across scopes (#4455)
Browse files Browse the repository at this point in the history
Fixes #4438

Due to:
- breadth-first type checking
- simplistic inference
- no re-checking (good)

Currently, this works:

```
let f = (arg) => {
  return arg.get("a");
};

if true {
  let x = f({ a: true });
}
```

but this does not work:

```
let f = () => {
  return { a: true };
};

if true {
  let x = f().get("a");
//        ^^^ don't know what the return type is yet because the closure above hasn't be checked
}
```

Because when first implementing inference, the sub-scopes of a scope were checked in reverse order. I thought this heuristic was kinda neat and figured it had legs. Now I'm thinking that was wrong. 

With this PR, the scope will be checked in normal order. Now the second case will work and the first case above will **not** work. It will require explicit annotation on the argument:

```
let f = (arg: Json) => {
  return arg.get("a");
};

if true {
  let x = f({ a: true });
}
```

*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)*.

BREAKING CHANGE: Inference now prioritizes earlier scopes rather than later ones. This previously allowed closures called from other scopes to leave off argument type annotations. [This](https://github.com/winglang/wing/blob/fcfc3462d37a063d96d49b4105e27466ff7793c8/examples/tests/invalid/inference.test.w#L106) is an example of a case that previously worked.
  • Loading branch information
MarkMcCulloh authored Oct 8, 2023
1 parent 31c8698 commit 8b6f604
Show file tree
Hide file tree
Showing 6 changed files with 241 additions and 172 deletions.
16 changes: 16 additions & 0 deletions examples/tests/invalid/inference.test.w
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,19 @@ let badFunc: inflight (str): void = inflight (arg1: num) => {};
// ^ Inferred type Array<str> conflicts with already inferred type Array<num>
};


let returnsNumber = () => { return 2; };
if true {
let x: str = returnsNumber();
// ^^^^^^^^^^^^^^^ Expected str, got num
}

// See https://github.com/winglang/wing/issues/4464 for details
let unknownArg = (arg) => {
// ^^^ Unable to infer type
return arg.get("a");
// ^^^ Property not found
};
if true {
unknownArg({ a: true });
}
12 changes: 7 additions & 5 deletions examples/tests/valid/inference.test.w
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,21 @@ clonedSet.add(4);

let api = new cloud.Api();
let func = inflight (request) => {
return cloud.ApiResponse {
return {
body: request.body,
status: 200,
};
};
api.get("/hello/world", func);

let argReturn = (n: num) => { return n; };
let implicitReturn = () => { return 1; };
if true {
api.get("/hello/world", func);
let a = argReturn(1);
let b = implicitReturn();
}

let returnsString = () => {
return "hi";
};
let returnsString = () => { return "hi"; };
let shouldBeString = returnsString();

let stringArray = [shouldBeString];
Expand Down
17 changes: 5 additions & 12 deletions libs/wingc/src/type_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use wingii::fqn::FQN;
use wingii::type_system::TypeSystem;

use self::class_fields_init::VisitClassInit;
use self::inference_visitor::InferenceVisitor;
use self::inference_visitor::{InferenceCounterVisitor, InferenceVisitor};
use self::jsii_importer::JsiiImportSpec;
use self::lifts::Lifts;
use self::symbol_env::{LookupResult, LookupResultMut, SymbolEnvIter, SymbolEnvRef};
Expand Down Expand Up @@ -1691,13 +1691,8 @@ impl<'a> TypeChecker<'a> {
/// Recursively check if a type is or contains a type inference.
///
/// Returns true if any inferences were found.
fn check_for_inferences(&mut self, node: &TypeRef, span: &WingSpan) -> bool {
let mut visitor = InferenceVisitor {
types: self.types,
found_inference: false,
expected_type: None,
span,
};
fn check_for_inferences(&self, node: &TypeRef) -> bool {
let mut visitor = InferenceCounterVisitor::default();

visitor.visit_typeref(node);

Expand Down Expand Up @@ -3024,9 +3019,7 @@ impl<'a> TypeChecker<'a> {
self.type_check_statement(statement, &mut env);
}

// reverse list to type check later scopes first
// Why? To improve the inference algorithm. Earlier inner_scopes for closures may need to infer types from later inner_scopes
let inner_scopes = self.inner_scopes.drain(..).rev().collect::<Vec<_>>();
let inner_scopes = self.inner_scopes.drain(..).collect::<Vec<_>>();
for (inner_scope, ctx) in inner_scopes {
let scope = unsafe { &*inner_scope };
self.ctx = ctx;
Expand All @@ -3052,7 +3045,7 @@ impl<'a> TypeChecker<'a> {

// If we found a variable with an inferred type, this is an error because it means we failed to infer its type
// Ignores any transient (no file_id) variables e.g. `this`. Those failed inferences are cascading errors and not useful to the user
if self.check_for_inferences(&var_info.type_, &var_info.name.span) && !var_info.name.span.file_id.is_empty() {
if !var_info.name.span.file_id.is_empty() && self.check_for_inferences(&var_info.type_) {
self.spanned_error(&var_info.name, "Unable to infer type".to_string());
}
}
Expand Down
29 changes: 29 additions & 0 deletions libs/wingc/src/type_check/inference_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,32 @@ impl<'a> crate::visit_types::VisitType<'_> for InferenceVisitor<'a> {
}
}
}

#[derive(Default)]
/// Deeply visits a type and finds any inferences.
pub struct InferenceCounterVisitor {
/// Whether or not we found an inference during the entire visit
pub found_inference: bool,

/// Whether or not we found an inference during the entire visit
pub found_inferences: Vec<InferenceId>,
}

impl crate::visit_types::VisitType<'_> for InferenceCounterVisitor {
// structs and interfaces cannot have inferences
fn visit_interface(&mut self, _node: &'_ Interface) {}
fn visit_struct(&mut self, _node: &'_ Struct) {}

fn visit_class(&mut self, node: &'_ Class) {
// We only care about visiting a class if it represent an inflight closure, where inference is possible.
// In which case, we need to visit the function signature of the handle method
if let Some(method) = node.get_closure_method() {
self.visit_typeref(&method);
}
}

fn visit_inference(&mut self, node: &'_ InferenceId) {
self.found_inference = true;
self.found_inferences.push(*node);
}
}
Loading

0 comments on commit 8b6f604

Please sign in to comment.