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

Unable to capture preflight object from within an inflight class #2730

Open
eladb opened this issue May 31, 2023 · 5 comments · May be fixed by #5559
Open

Unable to capture preflight object from within an inflight class #2730

eladb opened this issue May 31, 2023 · 5 comments · May be fixed by #5559
Assignees
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler

Comments

@eladb
Copy link
Contributor

eladb commented May 31, 2023

I tried this:

bring cloud;

let b = new cloud.Bucket();
let myConst = "bang bang";

inflight class Foo {
  uploadToBucket(k: str, value: str) {
    b.put(k, value);
  }
}

test "inflight class captures preflight resource" {
  let f = new Foo();
  f.uploadToBucket("hello.txt", "world");
}

This happened:

     │ Error: Missing environment variable: BUCKET_HANDLE_ed7b9545

I expected this:

To work

Is there a workaround?

No response

Component

Language Design

Wing Version

No response

Wing Console 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 May 31, 2023
@eladb eladb mentioned this issue Jun 2, 2023
5 tasks
mergify bot pushed a commit that referenced this issue Jun 2, 2023
## Description

This PR adds support for inflight classes:

```js
// either declared in a preflight scope
inflight class User {
  name: str;
  last: str;
  
  fullName(): str { return "${name} ${last}"; }
}
```

Inflight classes can also be declared within inflight scopes:

```js
class Foo {
  inflight bar() {
    class MyClass { } // inflight is implicit
    new MyClass();
  }
}
```

The `inflight` modifier is inherited from the scope, so there is no need
to add it when a class is defined within an inflight scope. Similarly, it is
required to specify `inflight` for members of an inflight class. All of them are
implicitly inflight.

## Implementation Notes

* This is a pretty ambitious refactor to merge the resources and classes in our type system (and jsification) into a single concept of "class" associated with a phase.
* When a class is declared without an explicit phase, it will inherit the phase of its context. This means that a `class` within an inflight context is the same as `inflight class` from a preflight context. This required tracking the phase during parsing (which is mostly just annoying boilerplate).
* I've attempted to also unify the code paths within the jsification phase so basically every inflight class is emitted into a separate file (same as the inflight portion of preflight classes) and rendered within the same code path with a few differences:
* Inflight initializer of inflight classes is basically `constructor` (while the preflight constructor is not emitted).
* When we emit an inflight class within an inflight context, we still emit it into a separate file (with capturing and all) and `require` this file at the point of declaration.

## Misc

* Cleaned up a bunch of whitespace issues in emitted JavaScript.
* I've flattened all of the emitted javascript output (removed the `clients/` directory) and switched the inflight files to have an `inflight.` prefix (instead of a suffix). This results in a nice list of JavaScript files - one called `preflight.js` and the rest are `inflight.Xyz.js`. This makes it a little cleaner to `require()` modules of inflight classes and gets rid of the concept of "client".
* Got rid of `Initializer` and `MethodLike` (because now initializers are just functions).
* Merged `FreeVariableScanner` and `PreflightTypeRefVisitor` into `CaptureScanner` that takes care of both.
* This PR includes #2737
* Added static `phase` tracking within the parser. This is required in order to determine the default implicit phase of declarations.
* Fixes #2413

## Follow ups
* We have a a problem with our async programming model in inflight classes because there is no way to indicate that a method is async. For now, all methods are async (since they are tagged `inflight`), but that means we cannot call them from within the constructor. We need to decide how to deal with it.
* Merge the `FieldReferenceVisitor` into `CaptureScanner` as well.
* #2764
* #2763
* #2757
* #2753
* #2730
* #2765


## 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
- [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 [Monada Contribution License](https://docs.winglang.io/terms-and-policies/contribution-license.html)*.
@skorfmann
Copy link
Contributor

Just ran into this

@github-actions
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!

@github-actions github-actions bot added the Stale label Aug 16, 2023
@yoav-steinberg yoav-steinberg self-assigned this Aug 21, 2023
@yoav-steinberg
Copy link
Collaborator

note to self: the cause of the issue is that the expression f above is an inflight expression (because it references an inflight object). Inflight expressions aren't lifted so the compiler doesn't store binding information (add_lift) for the call expression f.uploadToBucket. This in turn means that we don't setup access to b or "lift" it when setting up the test.

@hasanaburayyan
Copy link
Collaborator

Another related example:

inflight class Foo {
  inflight foo: cloud.Secret;
  new(foo: cloud.Secret) {
    this.foo = foo;
  }

  inflight do() {
    this.foo.value();
  }
}
❯ wing compile main.w 
error: Expression of type "Secret" references an unknown preflight object, can't qualify its capabilities. Use `lift()` to explicitly qualify the preflight object to disable this error.
   --> main.w:10:5
   |
10 |     this.foo.value();
   |     ^^^^^^^^

@Chriscbr
Copy link
Contributor

Just wanted to add some more context to @hasanaburayyan's comment. Even though the current compiler error message is offers a workaround for some Wing applications, it seems to be a problem for Wing libraries where we don't know which preflight object is going to be lifted ahead of time.

In the example below, Slack has an inflight method postMessage() which therein creates an inflight class Foo. But the secret object passed to Foo's constructor is an object provided by the consumer of the library, so it's not possible to add a lift() statement to silence the compiler error.

inflight class Foo {
  inflight s: cloud.Secret;
  new(s: cloud.Secret) {
    this.s = s;
  }

  inflight getValue(): str {
    return this.foo.value(); // error: Expression of type "Secret" references an unknown preflight object
  }
}

class Slack {
  secret: cloud.Secret;
  new(secret: cloud.Secret) {
    this.secret = secret;
  }

  inflight postMessage() {
    let foo = new Foo(this.secret);
    let apiKey = foo.getValue();
  }
}

// library usage
let s = new cloud.Secret();
new Slack(s);

It's worth noting that the limitation here parallels the existing limitations with passing objects through to inflight functions:

let getValue = inflight (s: cloud.Secret): str => {
  return s.value(); // error: Expression of type "Secret" references an unknown preflight object
};

class Slack {
  secret: cloud.Secret;
  new(secret: cloud.Secret) {
    this.secret = secret;
  }

  inflight postMessage() {
    let apiKey = getValue(this.secret);
  }
}

// library usage
let s = new cloud.Secret();
new Slack(s);

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
Projects
Status: 🏗 In progress
Development

Successfully merging a pull request may close this issue.

6 participants