Skip to content

Commit

Permalink
fix(compiler): can't instantiate preflight classes in static methods (#…
Browse files Browse the repository at this point in the history
…6040)

Fixed #6019
The fix adds an implicit `$scope` parameter to static methods so when they need to instantiate a preflight class they know from within which context they're being called.

Note the following exceptions to adding the implicit scope param to a static preflight method:
1) JSII imported methods. In the future we might want to support this via #6067.
2) `extern` methods, since the user implements these in JS/TS and doesn't expect an implicit scope parameter.
3) Static methods that override a method of the same name in a parent class that doesn't have an implicit scope argument (because one of the above reasons). We don't have clear definitions of static method overriding, but currently we do use this for overrding `onLiftType` inherited from the imported base `std.Resource` class. Opting out of implicit scoping for this case seemed like the easiest solution.

## 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)*.
  • Loading branch information
yoav-steinberg authored Apr 2, 2024
1 parent f74c6d7 commit b955422
Show file tree
Hide file tree
Showing 15 changed files with 366 additions and 86 deletions.
4 changes: 4 additions & 0 deletions examples/jsii-fixture/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ export class JsiiClass {
return `Got ${arg}`;
}

public static staticMethod(arg: string) {
return `Got ${arg}`;
}

public methodWithStructParam(s: SomeStruct): string {
return s.field;
}
Expand Down
15 changes: 0 additions & 15 deletions examples/tests/invalid/new_in_static_without_scope.test.w

This file was deleted.

53 changes: 51 additions & 2 deletions examples/tests/valid/new_in_static.test.w
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
bring cloud;
bring "constructs" as c;
bring "jsii-fixture" as jsii_fixture;

class MyClass {
pub static createBucket(scope: c.IConstruct): cloud.Bucket {
Expand All @@ -11,6 +12,25 @@ class MyClass {
// wing class
return new MyClass() in scope;
}

pub static createMyClassWithImplicitScope(id: str): MyClass {
return new MyClass() as "implicit-scope-myclass-{id}";
}

pub static createBucketWithImplicitScope(): cloud.Bucket {
return new cloud.Bucket() as "implicit-scope-bucket";
}

pub instanceMethod() {
// Implicit scope should be the instance's
let my = MyClass.createMyClassWithImplicitScope("from-instance-method");
let bucket = MyClass.createBucketWithImplicitScope();
}

pub static staticMehtodThatCallsAnotherStaticMethod(): MyClass {
return MyClass.createMyClassWithImplicitScope("from-outer-static-method");
}

}

// these should work
Expand All @@ -27,7 +47,36 @@ let scope = new c.Construct();
let bucket = MyClass.createBucket(scope);
let bucket2 = createBucket();
let my = MyClass.createMyClass(scope);
let my2 = MyClass.createMyClassWithImplicitScope("from-root");
let bucket3 = MyClass.createBucketWithImplicitScope();
let my3 = MyClass.staticMehtodThatCallsAnotherStaticMethod();

test "play with bucket" {
// Call instance that instantiates stuff using static methods (should use the instance's scope)
my.instanceMethod();

// Test calling a static JSII method (no implicit scope should be passed)
assert(jsii_fixture.JsiiClass.staticMethod("foo") == "Got foo");

test "play with buckets" {
assert(bucket.list().length == 0);
}
assert(bucket3.list().length == 0);
}

// Call static method from a class's ctor
class FooParent {
field: MyClass?;
new(myclass: MyClass?) {
this.field = myclass;
}
}
class Foo extends FooParent {
new() {
super(
// TODO: unsupported yet, see https://github.com/winglang/wing/issues/6016
// MyClass.createMyClassWithImplicitScope("from-ctor-before-super")
nil
);
MyClass.createMyClassWithImplicitScope("from-ctor");
}
}
let f = new Foo();
116 changes: 98 additions & 18 deletions libs/wingc/src/jsify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::{
is_udt_struct_type,
lifts::{LiftQualification, Liftable, Lifts},
resolve_super_method, resolve_user_defined_type,
symbol_env::SymbolEnv,
symbol_env::{SymbolEnv, SymbolEnvKind},
ClassLike, Type, TypeRef, Types, CLASS_INFLIGHT_INIT_NAME,
},
visit_context::{VisitContext, VisitorWithContext},
Expand Down Expand Up @@ -56,6 +56,8 @@ const NODE_MODULES_SCOPE_SPECIFIER: &str = "@";

const SUPER_CLASS_INFLIGHT_INIT_NAME: &str = formatcp!("super_{CLASS_INFLIGHT_INIT_NAME}");

const SCOPE_PARAM: &str = "$scope";

pub struct JSifyContext<'a> {
pub lifts: Option<&'a Lifts>,
pub visit_ctx: &'a mut VisitContext,
Expand Down Expand Up @@ -190,8 +192,8 @@ impl<'a> JSifier<'a> {
if is_entrypoint {
let mut root_class = CodeMaker::default();
root_class.open(format!("class {} extends {} {{", ROOT_CLASS, STDLIB_CORE_RESOURCE));
root_class.open(format!("{JS_CONSTRUCTOR}($scope, $id) {{"));
root_class.line("super($scope, $id);");
root_class.open(format!("{JS_CONSTRUCTOR}({SCOPE_PARAM}, $id) {{"));
root_class.line(format!("super({SCOPE_PARAM}, $id);"));
root_class.add_code(self.jsify_struct_schemas());
root_class.add_code(js);
root_class.close("}");
Expand Down Expand Up @@ -498,7 +500,17 @@ impl<'a> JSifier<'a> {
if let Some(scope) = obj_scope {
Some(self.jsify_expression(scope, ctx).to_string())
} else {
Some("this".to_string())
// If the current method has an implicit scope arg then use it, if not we can assume `this` is available
if ctx.visit_ctx.current_method_env().map_or(false, |e| {
let SymbolEnvKind::Function { sig, .. } = e.kind else {
panic!("Method env not a function env");
};
sig.as_function_sig().unwrap().implicit_scope_param
}) {
Some(SCOPE_PARAM.to_string())
} else {
Some("this".to_string())
}
}
} else {
None
Expand Down Expand Up @@ -528,7 +540,7 @@ impl<'a> JSifier<'a> {
Some(scope) => Some(if scope == "this" {
"this".to_string()
} else {
"$scope".to_string()
SCOPE_PARAM.to_string()
}),
}
};
Expand All @@ -547,7 +559,7 @@ impl<'a> JSifier<'a> {
if node_scope != "this" {
new_code!(
expr_span,
"($scope => $scope.node.root.new(\"",
format!("({SCOPE_PARAM} => {SCOPE_PARAM}.node.root.new(\""),
fqn,
"\", ",
ctor,
Expand Down Expand Up @@ -654,7 +666,8 @@ impl<'a> JSifier<'a> {
CalleeKind::Expr(expr) => self.jsify_expression(expr, ctx).to_string(),
CalleeKind::SuperCall(method) => format!("super.{}", method),
};
let args_string = self.jsify_arg_list(&arg_list, None, None, ctx).to_string();
let mut args_string = self.jsify_arg_list(&arg_list, None, None, ctx).to_string();

let mut args_text_string = lookup_span(&arg_list.span, &self.source_files);
if args_text_string.len() > 0 {
// remove the parens
Expand Down Expand Up @@ -691,6 +704,31 @@ impl<'a> JSifier<'a> {
let ac = AhoCorasick::new(patterns).expect("Failed to create macro pattern");
return new_code!(expr_span, ac.replace_all(js_override, replace_with));
}

// If this function requires an implicit scope argument, we need to add it to the args string
if function_sig.implicit_scope_param {
// If the current function we're in also has an implicit scope parameter then use it
// TODO: make a helper function to get the `current_function_type`
let implicit_scope_arg_available = ctx.visit_ctx.current_function_env().map_or(false, |e| {
if let SymbolEnvKind::Function { sig, .. } = e.kind {
sig.as_function_sig().expect("a function sig").implicit_scope_param
} else {
false
}
});

let prepend_scope_arg = if implicit_scope_arg_available {
SCOPE_PARAM.to_string()
} else {
// Otherwise, we can just use `this`. We can assume `this` is available since othesize we should have had an implicit scope arg available.
"this".to_string()
};
if args_string.len() > 0 {
args_string = format!("{}, {}", prepend_scope_arg, args_string);
} else {
args_string = prepend_scope_arg;
}
}
}

let optional_access = if is_option { "?." } else { "" };
Expand Down Expand Up @@ -947,7 +985,12 @@ impl<'a> JSifier<'a> {
args,
");"
)),
Phase::Preflight => code.line(new_code!(&arg_list.span, "super($scope, $id, ", args, ");")),
Phase::Preflight => code.line(new_code!(
&arg_list.span,
format!("super({SCOPE_PARAM}, $id, "),
args,
");"
)),
Phase::Independent => {
// If our parent is phase independent then we don't call its super, instead a call to its super will be
// generated in `jsify_inflight_init` when we generate the inflight init for this class.
Expand Down Expand Up @@ -1303,20 +1346,50 @@ impl<'a> JSifier<'a> {

fn jsify_function(
&self,
class: Option<&AstClass>,
class_type: Option<TypeRef>,
func_def: &FunctionDefinition,
is_closure: bool,
ctx: &mut JSifyContext,
) -> CodeMaker {
let parameters = jsify_function_parameters(func_def);

// If this function requires an implicit scope parameter then add it to the parameters
let parameters = if class_type.map_or(false, |t| {
t.as_class()
.unwrap()
.get_method(func_def.name.as_ref().unwrap())
.unwrap()
.type_
.as_function_sig()
.unwrap()
.implicit_scope_param
}) {
let mut res = CodeMaker::one_line(SCOPE_PARAM);
if !parameters.is_empty() {
res.append(", ");
res.append(parameters);
}
res
} else {
parameters
};

let (name, arrow) = match &func_def.name {
Some(name) => (name.name.clone(), " ".to_string()),
None => ("".to_string(), " => ".to_string()),
};

let body = match &func_def.body {
FunctionBody::Statements(scope) => self.jsify_scope_body(scope, ctx),
FunctionBody::Statements(scope) => {
let function_env = self.types.get_scope_env(&scope);
ctx.with_function_def(
func_def.name.as_ref(),
&func_def.signature,
func_def.is_static,
function_env,
|ctx| self.jsify_scope_body(scope, ctx),
)
}
FunctionBody::External(extern_path) => {
let entrypoint_is_file = self.compilation_init_path.is_file();
let entrypoint_dir = if entrypoint_is_file {
Expand Down Expand Up @@ -1389,7 +1462,7 @@ impl<'a> JSifier<'a> {
};
let mut prefix = vec![];

if func_def.is_static && class.is_some() {
if func_def.is_static && class_type.is_some() {
prefix.push("static")
}

Expand Down Expand Up @@ -1439,7 +1512,7 @@ impl<'a> JSifier<'a> {
};

// emit the inflight side of the class into a separate file
let inflight_class_code = self.jsify_class_inflight(&class, ctx);
let inflight_class_code = self.jsify_class_inflight(class_type, &class, ctx);

// if this is inflight/independent, class, just emit the inflight class code inline and move on
// with your life.
Expand Down Expand Up @@ -1504,7 +1577,7 @@ impl<'a> JSifier<'a> {

// emit preflight methods
for m in class.preflight_methods(false) {
code.line(self.jsify_function(Some(class), m, false, ctx));
code.line(self.jsify_function(Some(class_type), m, false, ctx));
}

// emit the `_toInflight` and `_toInflightType` methods (TODO: renamed to `_liftObject` and
Expand All @@ -1525,7 +1598,7 @@ impl<'a> JSifier<'a> {
let mut code = new_code!(
&class.name.span,
JS_CONSTRUCTOR,
"($scope, $id, ",
format!("({SCOPE_PARAM}, $id, "),
jsify_function_parameters(&class.initializer),
") {"
);
Expand All @@ -1548,9 +1621,16 @@ impl<'a> JSifier<'a> {
// we always need a super() call because even if the class doesn't have an explicit parent, it
// will inherit from core.Resource.
if !super_called {
body_code.line("super($scope, $id);");
body_code.line(format!("super({SCOPE_PARAM}, $id);"));
}
body_code.add_code(self.jsify_scope_body(&init_statements, ctx));
let init_code = ctx.with_function_def(
class.initializer.name.as_ref(),
&class.initializer.signature,
class.initializer.is_static,
self.types.get_scope_env(init_statements),
|ctx| self.jsify_scope_body(init_statements, ctx),
);
body_code.add_code(init_code);

code.add_code(body_code);

Expand Down Expand Up @@ -1624,7 +1704,7 @@ impl<'a> JSifier<'a> {
}

// Write a class's inflight to a file
fn jsify_class_inflight(&self, class: &AstClass, mut ctx: &mut JSifyContext) -> CodeMaker {
fn jsify_class_inflight(&self, class_type: TypeRef, class: &AstClass, mut ctx: &mut JSifyContext) -> CodeMaker {
ctx.visit_ctx.push_phase(Phase::Inflight);

let mut class_code = new_code!(&class.name.span, "class ", &class.name.name);
Expand All @@ -1643,7 +1723,7 @@ impl<'a> JSifier<'a> {
}

for def in class.inflight_methods(false) {
class_code.line(self.jsify_function(Some(class), def, false, ctx));
class_code.line(self.jsify_function(Some(class_type), def, false, ctx));
}

// emit the $inflight_init function (if it has a body).
Expand Down
Loading

0 comments on commit b955422

Please sign in to comment.