Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't pass an inflight closure to a super ctor call #4925

Closed
eladb opened this issue Nov 14, 2023 · 5 comments · Fixed by #6599
Closed

Can't pass an inflight closure to a super ctor call #4925

eladb opened this issue Nov 14, 2023 · 5 comments · Fixed by #6599
Assignees
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler 🐶 dogfood Discovered while dogfooding Winglang

Comments

@eladb
Copy link
Contributor

eladb commented Nov 14, 2023

I tried this:

class Base {
  new(handler: inflight (): void) { }
}

class Derived extends Base {
  new() {
    super(inflight () => { });
  }
}

new Derived();

This happened:

➜  winglibs git:(main) wing compile ~/Documents/test.main.w 
ERROR: There is already a Construct with name 'Derived' in $Root [Default]

hint: Every preflight object needs a unique identifier within its scope. You can assign one as shown:

> new cloud.Bucket() as "MyBucket";

For more information, see https://www.winglang.io/docs/concepts/application-tree

../../Documents/target/test.main.wsim.543615.tmp/.wing/preflight.js:11
       class Base extends $stdlib.std.Resource {
         constructor($scope, $id, handler) {
>>         super($scope, $id);
         }
         static _toInflightType(context) {

I expected this:

To work

Is there a workaround?

No response

Component

Compiler

Wing Version

No response

Node.js Version

No response

Platform(s)

No response

Anything else?

No response

Community Notes

  • Please vote by adding a 👍 reaction to the issue to help us prioritize.
  • If you are interested to work on this issue, please leave a comment.
@eladb eladb added the 🐛 bug Something isn't working label Nov 14, 2023
@monadabot monadabot added this to Wing Nov 14, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New - not properly defined in Wing Nov 14, 2023
@staycoolcall911 staycoolcall911 changed the title There is already a Construct with name 'Derived' in $Root [Default] There is already a Construct with name 'Derived' in $Root [Default] Nov 14, 2023
@staycoolcall911 staycoolcall911 moved this from 🆕 New - not properly defined to 🤝 Backlog - handoff to owners in Wing Nov 14, 2023
Copy link

Hi,

This issue hasn't seen activity in 60 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days.
Feel free to re-open this issue when there's an update or relevant information to be added.
Thanks!

@staycoolcall911
Copy link
Contributor

staycoolcall911 commented Jan 21, 2024

Another simpler case of this (by EladB):

class Foo {
  pub bar() {
    inflight () => {};
  }
}

let f = new Foo();
f.bar();
f.bar();

Update: this issue is a different issue and is a dup of #2853. See details for specific issue below.

@yoav-steinberg
Copy link
Contributor

yoav-steinberg commented Jan 21, 2024

Detailed problem description

Passing an inflight closure to a super ctor call doesn't currently work for the following reasons:

  1. We currently auto add a super call when we jsify the ctor if the first statement isn't already a super call. This fails in our case because the first statement is actually the transformed inflight closure into a class definition. So we don't detect the existing call to super (in the example above) and end up with two super calls.
  2. The inflight closure transformer adds code that references this. But because we reference this before calling super we end up with invalid JS code (super must be called before using this in ctors). These are the reasons we reference this in transformed inflight closures:
    • 2.1. For setting up the parent_this used to replace this references inside the closure.
    • 2.2. When instantiating the instance of the closure class we need to pass in a scope argument and pass in this as the scope.

Thoughts about fixing this

One alternative is getting rid of inflight closure transformation altogether (see #5033). But if we want to avoid that at this point here are some other thoughts:

  • Re 1. above: we can detect the transformed inflight (and perhaps all type definitions) before the super call and not blindly look at the first statement when searching for a super call.
  • Re 2.2 above: we can pass a non this scope into the generated closure class. It can probably be any scope.
  • Re 2.1 above: the closure transformer can check if there are any references to this in the closure code. If there aren't we don't need parent_this anyway. If there are we can fail compilation in case this closure is used inside a ctor before the super call statement.

@yoav-steinberg yoav-steinberg changed the title There is already a Construct with name 'Derived' in $Root [Default] Can't pass an inflight closure to a super ctor call Jan 23, 2024
mergify bot pushed a commit that referenced this issue Jan 23, 2024
See #4925
We can now code-gen inflight closures without construct id conflicts.

## 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)
- [x] Docs updated (only required for features)
- [x] 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)*.
@Chriscbr Chriscbr added this to the Winglang Stable Release milestone Feb 27, 2024
@Chriscbr
Copy link
Contributor

Related: #6387

mergify bot pushed a commit that referenced this issue May 22, 2024
…t used (#6531)

This is a small code cleanup as part of addressing #4925.

We only generate the `parent_this` variable used in generated closure classes when needed. If the inflight closure never accesses `this` then we don't generated `parent_this`. The code avoids traversing into inner classes inside the closure when searching for `this` accesses, making it a bit cleaner and more efficient.

This relates to #4925 because it solves the problem of accessing `this` (for assignment into `parent_this`) before the `super()` call in a ctor.

## 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
- [ ] 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)*.
mergify bot pushed a commit that referenced this issue May 24, 2024
…6535)

This is part of the effort to address #4925.

Because inflight closures turn into class definitions, when they are passed to a `super()` call we use to get an error telling us that `super()` must be the first statement in a ctor. Now this is fixed:
```wing
class Foo {
  new(x: num) {}
}
class Bar extends Foo {
  new() {
    class Baz {} // This is now ok
    super(1);
  }
}
```
This PR also improves the diagnostics for mssing super calls or "not first statement" super calls.

note: waiting for #6531 to be merged first.

## 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)*.
@mergify mergify bot closed this as completed in #6599 Jun 2, 2024
mergify bot pushed a commit that referenced this issue Jun 2, 2024
…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)*.
@github-project-automation github-project-automation bot moved this from 🤝 Backlog - handoff to owners to ✅ Done in Wing Jun 2, 2024
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.74.30.

eladb pushed a commit that referenced this issue Jun 6, 2024
…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)*.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler 🐶 dogfood Discovered while dogfooding Winglang
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants