From ccdd51bbe22597143a26ae822b06d07ef3942cc5 Mon Sep 17 00:00:00 2001 From: yoav-steinberg Date: Sun, 2 Jun 2024 09:11:06 +0300 Subject: [PATCH] fix(compiler): can't pass inflight closure as parameter to `super()` call (#6599) fixes #4925 Now you can pass inflight closures and also any `new PreflightClass()` as parameters to a `super()` call in a ctor. What I did is detect these cases and made sure the implicit scope passed to the construct ctor is `$scope` and not `this` which isn't available before `super()` is called. ## 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)*. --- docs/docs/02-concepts/02-application-tree.md | 116 +++++++ .../inflight_closure_as_super_param.test.w | 35 +++ libs/wingc/src/jsify.rs | 11 +- libs/wingc/src/lifting.rs | 2 +- libs/wingc/src/lsp/symbol_locator.rs | 2 +- libs/wingc/src/type_check.rs | 2 +- libs/wingc/src/visit_context.rs | 25 +- ...re_as_super_param.test.w_compile_tf-aws.md | 286 ++++++++++++++++++ ..._closure_as_super_param.test.w_test_sim.md | 12 + 9 files changed, 480 insertions(+), 11 deletions(-) create mode 100644 examples/tests/valid/inflight_closure_as_super_param.test.w create mode 100644 tools/hangar/__snapshots__/test_corpus/valid/inflight_closure_as_super_param.test.w_compile_tf-aws.md create mode 100644 tools/hangar/__snapshots__/test_corpus/valid/inflight_closure_as_super_param.test.w_test_sim.md diff --git a/docs/docs/02-concepts/02-application-tree.md b/docs/docs/02-concepts/02-application-tree.md index d1536113e34..dfa9c2079a1 100644 --- a/docs/docs/02-concepts/02-application-tree.md +++ b/docs/docs/02-concepts/02-application-tree.md @@ -3,6 +3,8 @@ id: application-tree title: Application tree --- +## Instance names + Instances of preflight classes in Wing are identified by a unique name. The name is used to identify the resource in the Wing Console, and is used to determine the logical name assigned to the resource in the infrastructure provisioning engine (such as Terraform or CloudFormation), and the physical name of the resource in the target cloud provider (such as AWS, Azure, or GCP). @@ -34,6 +36,120 @@ new Group1(); new Group2(); ``` +## Instance scope + +Instances of preflight classes define the application's construct tree. This way you can think about the generated cloud infrastructure as a logical tree where each node is some logical piece of infrastructure which may contain children nodes which are also part of your application's infrastructure (composition). + +In Wing this tree structure is automatically generated. Any class instantiated at the top level (global scope) is a child of the "root" node of of the tree. +Any class instantiated inside another class (in its constructor or one of its other preflight methods) will be placed as a child of that other class. + +```js +class ThumbnailBucket { + //... +} + +class ImageStorage { + new() { + new ThumbnailBucket(); // This ThumbnailBucket will be a child of a ImageStorage instance in the construct tree + } +} + +new ImageStorage(); // This ImageStorage will be a child of the root in the construct tree +new ThumbnailBucket(); // This Counter will be a child of of the root in the construct tree + +// Here's a tree view of the generated infrastructure: +// +// root +// /\ +// ImageStorage ThumbnailBucket +// / +// ThumbnailBucket +``` + +As mentioned in the [previous section](#instance-names) each instance must have a unique name within its scope. +And the name will be automatically generated based on the class name. +So the tree shown above also shows the correct names for each infrastructure piece of our application. +You may query information about the construct tree using the `nodeof(someInstance)` intrinsic function: +```js +let b = new cloud.Bucket(); +log(nodeof(b).path); // Will log something like "/root/Bucket" +``` + +### Controlling instance scope + +You may define an explicit scope for an instance instead of using Wing's default of placing it inside the instance where it was created using the `in` keyword: + +```js +class ThumbnailBucket { + //... +} + +class ImageStorage { + new() { + new ThumbnailBucket(); // This ThumbnailBucket will be a child of a ImageStorage instance in the construct tree + } +} + +let imageStorage = new ImageStorage(); // This goes in root +let defaultThumbnails = new ThumbnailBucket() as "defaultThumbs" in imageStorage; // This is explicitly named "defaultThumbs" and explicitly placed inside imageStorage + +// Here's a tree view of the generated infrastructure: +// +// root +// / +// ImageStorage +// / \ +// ThumbnailBucket defaultsThumbs +``` + +### Instances created inside static methods + +Preflight classes instantiated inside static methods, or instantiated inside constructors before `this` is available will use the +scope of the caller by default: + +```js +class Factory { + pub static make() { + new cloud.Bucket(); // We're in a static, so we don't know where to place this bucket + } +} + +class MyBucket() { + new() { + Factory.make(); // Bucket will be placed inside `this` instance of `MyBucket` + } +} +new MyBucket(); + +// tree: +// root +// / +// MyBucket +// / +// Bucket +``` + +Similarly, consider this case where we instantiate a class inside a parameter in a `super()` constructor call before the +the class is creates and its scope becomes valid: + +```js +class Base { + new(b: cloud.Bucket) {} +} +class Derived extends Base { + new() { + super(new cloud.Bucket()); // Bucket create before `Derived` so scope defaults to caller + } +} +new Derived(); + +// tree: +// root +// /\ +// Bucket Derived +``` + +## Interop Classes in Wing are an extension of the [Construct Programming Model] and as such any [AWS Constructs] can be natively used in Wing applications. [Construct Programming Model]: https://docs.aws.amazon.com/cdk/v2/guide/constructs.html diff --git a/examples/tests/valid/inflight_closure_as_super_param.test.w b/examples/tests/valid/inflight_closure_as_super_param.test.w new file mode 100644 index 00000000000..8cf60c60a19 --- /dev/null +++ b/examples/tests/valid/inflight_closure_as_super_param.test.w @@ -0,0 +1,35 @@ +class Foo {} + +class Base { + pub h: inflight (): str; + pub f_base: Foo; + new(handler: inflight (): str, f: Foo) { + this.h = handler; + this.f_base = f; + } +} + +class Derived extends Base { + pub f: Foo; + new() { + super(inflight (): str => { + return "boom!"; + }, new Foo() as "in_root"); + this.f = new Foo() as "in_derived"; + } +} + +let c = new Derived() as "derived"; +// Make sure that instances created in a ctor are scoped to the instance they were created in +// This is related to this test because the transformed inflight closure is also an instance +// created in a ctor, but it's special cased to to be scoped in the class because `this` isn't +// available during the closures instantiation. +assert(nodeof(c.f).path.endsWith("derived/in_derived")); +// Make sure the instance created in the super call is scoped to the parent (root) +assert(!nodeof(c.f_base).path.endsWith("derived/in_root")); +let appPath = nodeof(this).path; +assert(nodeof(c.f_base).path == "{appPath}/in_root"); + +test "boom!" { + assert(c.h() == "boom!"); +} \ No newline at end of file diff --git a/libs/wingc/src/jsify.rs b/libs/wingc/src/jsify.rs index 848c820ac69..4441c6f3000 100644 --- a/libs/wingc/src/jsify.rs +++ b/libs/wingc/src/jsify.rs @@ -523,7 +523,14 @@ impl<'a> JSifier<'a> { }) { Some(SCOPE_PARAM.to_string()) } else { - Some("this".to_string()) + // By default use `this` as the scope. + // If we're inside an argument to a `super()` call then `this` isn't avialble, in which case + // we can safely use the ctor's `$scope` arg. + if ctx.visit_ctx.current_stmt_is_super_call() { + Some(SCOPE_PARAM.to_string()) + } else { + Some("this".to_string()) + } } } } else { @@ -1146,7 +1153,7 @@ impl<'a> JSifier<'a> { let mut code = CodeMaker::with_source(&statement.span); CompilationContext::set(CompilationPhase::Jsifying, &statement.span); - ctx.visit_ctx.push_stmt(statement.idx); + ctx.visit_ctx.push_stmt(statement); match &statement.kind { StmtKind::Bring { source, identifier } => match source { BringSource::BuiltinModule(name) => { diff --git a/libs/wingc/src/lifting.rs b/libs/wingc/src/lifting.rs index 244895603ce..3be3514185f 100644 --- a/libs/wingc/src/lifting.rs +++ b/libs/wingc/src/lifting.rs @@ -423,7 +423,7 @@ impl<'a> Visit<'a> for LiftVisitor<'a> { fn visit_stmt(&mut self, node: &'a Stmt) { CompilationContext::set(CompilationPhase::Lifting, &node.span); - self.ctx.push_stmt(node.idx); + self.ctx.push_stmt(node); // If this is an explicit lift statement then add the explicit lift if let StmtKind::ExplicitLift(explicit_lift) = &node.kind { diff --git a/libs/wingc/src/lsp/symbol_locator.rs b/libs/wingc/src/lsp/symbol_locator.rs index f0b2f6682e9..9bdbd0b0b20 100644 --- a/libs/wingc/src/lsp/symbol_locator.rs +++ b/libs/wingc/src/lsp/symbol_locator.rs @@ -225,7 +225,7 @@ impl<'a> Visit<'a> for SymbolLocator<'a> { return; } - self.ctx.push_stmt(node.idx); + self.ctx.push_stmt(node); // Handle situations where symbols are actually defined in inner scopes match &node.kind { diff --git a/libs/wingc/src/type_check.rs b/libs/wingc/src/type_check.rs index 1df2e4fc6d7..38227371819 100644 --- a/libs/wingc/src/type_check.rs +++ b/libs/wingc/src/type_check.rs @@ -4124,7 +4124,7 @@ new cloud.Function(@inflight("./handler.ts"), lifts: { bucket: ["put"] }); CompilationContext::set(CompilationPhase::TypeChecking, &stmt.span); // Set the current statement index for symbol lookup checks. - self.with_stmt(stmt.idx, |tc| match &stmt.kind { + self.with_stmt(stmt, |tc| match &stmt.kind { StmtKind::Let { reassignable, var_name, diff --git a/libs/wingc/src/visit_context.rs b/libs/wingc/src/visit_context.rs index 4915c352b3c..5004ba58074 100644 --- a/libs/wingc/src/visit_context.rs +++ b/libs/wingc/src/visit_context.rs @@ -1,7 +1,7 @@ use itertools::Itertools; use crate::{ - ast::{Class, ExprId, FunctionSignature, Phase, Symbol, UserDefinedType}, + ast::{Class, ExprId, FunctionSignature, Phase, Stmt, StmtKind, Symbol, UserDefinedType}, type_check::symbol_env::SymbolEnvRef, }; @@ -18,6 +18,12 @@ pub enum PropertyObject { Instance(ExprId), } +#[derive(Clone)] +pub struct StmtContext { + pub idx: usize, + pub super_call: bool, +} + #[derive(Clone)] pub struct VisitContext { phase: Vec, @@ -26,7 +32,7 @@ pub struct VisitContext { property: Vec<(PropertyObject, Symbol)>, function: Vec, class: Vec, - statement: Vec, + statement: Vec, in_json: Vec, in_type_annotation: Vec, expression: Vec, @@ -64,8 +70,11 @@ impl VisitContext { // -- - pub fn push_stmt(&mut self, stmt: usize) { - self.statement.push(stmt); + pub fn push_stmt(&mut self, stmt: &Stmt) { + self.statement.push(StmtContext { + idx: stmt.idx, + super_call: matches!(stmt.kind, StmtKind::SuperConstructor { .. }), + }); } pub fn pop_stmt(&mut self) { @@ -73,7 +82,11 @@ impl VisitContext { } pub fn current_stmt_idx(&self) -> usize { - *self.statement.last().unwrap_or(&0) + self.statement.last().map_or(0, |s| s.idx) + } + + pub fn current_stmt_is_super_call(&self) -> bool { + self.statement.last().map_or(false, |s| s.super_call) } // -- @@ -230,7 +243,7 @@ pub trait VisitorWithContext { self.ctx().pop_expr(); } - fn with_stmt(&mut self, stmt: usize, f: impl FnOnce(&mut Self)) { + fn with_stmt(&mut self, stmt: &Stmt, f: impl FnOnce(&mut Self)) { self.ctx().push_stmt(stmt); f(self); self.ctx().pop_stmt(); diff --git a/tools/hangar/__snapshots__/test_corpus/valid/inflight_closure_as_super_param.test.w_compile_tf-aws.md b/tools/hangar/__snapshots__/test_corpus/valid/inflight_closure_as_super_param.test.w_compile_tf-aws.md new file mode 100644 index 00000000000..dd11ada8e03 --- /dev/null +++ b/tools/hangar/__snapshots__/test_corpus/valid/inflight_closure_as_super_param.test.w_compile_tf-aws.md @@ -0,0 +1,286 @@ +# [inflight_closure_as_super_param.test.w](../../../../../examples/tests/valid/inflight_closure_as_super_param.test.w) | compile | tf-aws + +## inflight.$Closure1-1.cjs +```cjs +"use strict"; +const $helpers = require("@winglang/sdk/lib/helpers"); +module.exports = function({ }) { + class $Closure1 { + constructor({ }) { + const $obj = (...args) => this.handle(...args); + Object.setPrototypeOf($obj, this); + return $obj; + } + async handle() { + return "boom!"; + } + } + return $Closure1; +} +//# sourceMappingURL=inflight.$Closure1-1.cjs.map +``` + +## inflight.$Closure2-1.cjs +```cjs +"use strict"; +const $helpers = require("@winglang/sdk/lib/helpers"); +module.exports = function({ $c_h }) { + class $Closure2 { + constructor({ }) { + const $obj = (...args) => this.handle(...args); + Object.setPrototypeOf($obj, this); + return $obj; + } + async handle() { + $helpers.assert($helpers.eq((await $c_h()), "boom!"), "c.h() == \"boom!\""); + } + } + return $Closure2; +} +//# sourceMappingURL=inflight.$Closure2-1.cjs.map +``` + +## inflight.Base-1.cjs +```cjs +"use strict"; +const $helpers = require("@winglang/sdk/lib/helpers"); +module.exports = function({ }) { + class Base { + constructor({ }) { + } + } + return Base; +} +//# sourceMappingURL=inflight.Base-1.cjs.map +``` + +## inflight.Derived-1.cjs +```cjs +"use strict"; +const $helpers = require("@winglang/sdk/lib/helpers"); +module.exports = function({ $Base }) { + class Derived extends $Base { + constructor({ }) { + super({ }); + } + } + return Derived; +} +//# sourceMappingURL=inflight.Derived-1.cjs.map +``` + +## inflight.Foo-1.cjs +```cjs +"use strict"; +const $helpers = require("@winglang/sdk/lib/helpers"); +module.exports = function({ }) { + class Foo { + constructor({ }) { + } + } + return Foo; +} +//# sourceMappingURL=inflight.Foo-1.cjs.map +``` + +## main.tf.json +```json +{ + "//": { + "metadata": { + "backend": "local", + "stackName": "root", + "version": "0.20.3" + }, + "outputs": {} + }, + "provider": { + "aws": [ + {} + ] + } +} +``` + +## preflight.cjs +```cjs +"use strict"; +const $stdlib = require('@winglang/sdk'); +const $platforms = ((s) => !s ? [] : s.split(';'))(process.env.WING_PLATFORMS); +const $outdir = process.env.WING_SYNTH_DIR ?? "."; +const $wing_is_test = process.env.WING_IS_TEST === "true"; +const std = $stdlib.std; +const $helpers = $stdlib.helpers; +const $extern = $helpers.createExternRequire(__dirname); +class $Root extends $stdlib.std.Resource { + constructor($scope, $id) { + super($scope, $id); + class Foo extends $stdlib.std.Resource { + constructor($scope, $id, ) { + super($scope, $id); + } + static _toInflightType() { + return ` + require("${$helpers.normalPath(__dirname)}/inflight.Foo-1.cjs")({ + }) + `; + } + _toInflight() { + return ` + (await (async () => { + const FooClient = ${Foo._toInflightType()}; + const client = new FooClient({ + }); + if (client.$inflight_init) { await client.$inflight_init(); } + return client; + })()) + `; + } + get _liftMap() { + return ({ + "$inflight_init": [ + ], + }); + } + } + class Base extends $stdlib.std.Resource { + constructor($scope, $id, handler, f) { + super($scope, $id); + this.h = handler; + this.f_base = f; + } + static _toInflightType() { + return ` + require("${$helpers.normalPath(__dirname)}/inflight.Base-1.cjs")({ + }) + `; + } + _toInflight() { + return ` + (await (async () => { + const BaseClient = ${Base._toInflightType()}; + const client = new BaseClient({ + }); + if (client.$inflight_init) { await client.$inflight_init(); } + return client; + })()) + `; + } + get _liftMap() { + return ({ + "$inflight_init": [ + ], + }); + } + } + class Derived extends Base { + constructor($scope, $id, ) { + class $Closure1 extends $stdlib.std.AutoIdResource { + _id = $stdlib.core.closureId(); + constructor($scope, $id, ) { + super($scope, $id); + $helpers.nodeof(this).hidden = true; + } + static _toInflightType() { + return ` + require("${$helpers.normalPath(__dirname)}/inflight.$Closure1-1.cjs")({ + }) + `; + } + _toInflight() { + return ` + (await (async () => { + const $Closure1Client = ${$Closure1._toInflightType()}; + const client = new $Closure1Client({ + }); + if (client.$inflight_init) { await client.$inflight_init(); } + return client; + })()) + `; + } + get _liftMap() { + return ({ + "handle": [ + ], + "$inflight_init": [ + ], + }); + } + } + super($scope, $id, new $Closure1($scope, "$Closure1"), new Foo($scope, "in_root")); + this.f = new Foo(this, "in_derived"); + } + static _toInflightType() { + return ` + require("${$helpers.normalPath(__dirname)}/inflight.Derived-1.cjs")({ + $Base: ${$stdlib.core.liftObject(Base)}, + }) + `; + } + _toInflight() { + return ` + (await (async () => { + const DerivedClient = ${Derived._toInflightType()}; + const client = new DerivedClient({ + }); + if (client.$inflight_init) { await client.$inflight_init(); } + return client; + })()) + `; + } + get _liftMap() { + return $stdlib.core.mergeLiftDeps(super._liftMap, { + "$inflight_init": [ + ], + }); + } + } + class $Closure2 extends $stdlib.std.AutoIdResource { + _id = $stdlib.core.closureId(); + constructor($scope, $id, ) { + super($scope, $id); + $helpers.nodeof(this).hidden = true; + } + static _toInflightType() { + return ` + require("${$helpers.normalPath(__dirname)}/inflight.$Closure2-1.cjs")({ + $c_h: ${$stdlib.core.liftObject(c.h)}, + }) + `; + } + _toInflight() { + return ` + (await (async () => { + const $Closure2Client = ${$Closure2._toInflightType()}; + const client = new $Closure2Client({ + }); + if (client.$inflight_init) { await client.$inflight_init(); } + return client; + })()) + `; + } + get _liftMap() { + return ({ + "handle": [ + [c.h, ["handle"]], + ], + "$inflight_init": [ + [c.h, []], + ], + }); + } + } + const c = new Derived(this, "derived"); + $helpers.assert($helpers.nodeof(c.f).path.endsWith("derived/in_derived"), "nodeof(c.f).path.endsWith(\"derived/in_derived\")"); + $helpers.assert((!$helpers.nodeof(c.f_base).path.endsWith("derived/in_root")), "!nodeof(c.f_base).path.endsWith(\"derived/in_root\")"); + const appPath = $helpers.nodeof(this).path; + $helpers.assert($helpers.eq($helpers.nodeof(c.f_base).path, String.raw({ raw: ["", "/in_root"] }, appPath)), "nodeof(c.f_base).path == \"{appPath}/in_root\""); + this.node.root.new("@winglang/sdk.std.Test", std.Test, this, "test:boom!", new $Closure2(this, "$Closure2")); + } +} +const $PlatformManager = new $stdlib.platform.PlatformManager({platformPaths: $platforms}); +const $APP = $PlatformManager.createApp({ outdir: $outdir, name: "inflight_closure_as_super_param.test", rootConstruct: $Root, isTestEnvironment: $wing_is_test, entrypointDir: process.env['WING_SOURCE_DIR'], rootId: process.env['WING_ROOT_ID'] }); +$APP.synth(); +//# sourceMappingURL=preflight.cjs.map +``` + diff --git a/tools/hangar/__snapshots__/test_corpus/valid/inflight_closure_as_super_param.test.w_test_sim.md b/tools/hangar/__snapshots__/test_corpus/valid/inflight_closure_as_super_param.test.w_test_sim.md new file mode 100644 index 00000000000..9c4b92869cc --- /dev/null +++ b/tools/hangar/__snapshots__/test_corpus/valid/inflight_closure_as_super_param.test.w_test_sim.md @@ -0,0 +1,12 @@ +# [inflight_closure_as_super_param.test.w](../../../../../examples/tests/valid/inflight_closure_as_super_param.test.w) | test | sim + +## stdout.log +```log +pass ─ inflight_closure_as_super_param.test.wsim » root/env0/test:boom! + +Tests 1 passed (1) +Snapshots 1 skipped +Test Files 1 passed (1) +Duration +``` +