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

rfc: unphased functions #1711

Closed
wants to merge 17 commits into from
Closed

rfc: unphased functions #1711

wants to merge 17 commits into from

Conversation

Chriscbr
Copy link
Contributor

@Chriscbr Chriscbr commented Mar 3, 2023

This doc proposes how phase-independent functions should look and behave in Winglang. Related to #435

From a compiler perspective, these functions would essentially be code generated in both the preflight JS class and the inflight JS class.

By submitting this pull request, I confirm that my contribution is made under the terms of the Monada Contribution License.

@Chriscbr Chriscbr requested a review from a team as a code owner March 3, 2023 03:27
@Chriscbr
Copy link
Contributor Author

Chriscbr commented Mar 3, 2023

Another idea/suggestion is to designate these functions with the keyword pure

@hasanaburayyan
Copy link
Contributor

Im on board, I just dont much care for the prefixed ? maybe a keyword instead like pure but no strong opinions here.

@ShaiBer
Copy link
Contributor

ShaiBer commented Mar 5, 2023

I agree that letting the compiler infer whether a function is phase-independent is not the right way to go.
I also agree that making functions phase-independent should not be the default because it sends the wrong message about which type of function we expect to be more prevalent if writing correct wing code.

Like @hasanaburayyan, I'm also not crazy about the ?inflight keyword. I like Rust's approach with maybe-inflight better, but it also doesn't feel like the best solution to me. I hope someone will come up with a better alternative everyone will like.

I'm also OK with experimenting with having to always put a phase modifier on functions, and then we have preflight, inflight and maybe-inflight (or ?inflight or something better someone else will come up with)

@ekeren
Copy link

ekeren commented Mar 5, 2023

@Chriscbr , great doc
Can you also add the reasoning and need for phase-independent methods, have you came across any concrete use cases?

@marciocadev
Copy link
Collaborator

marciocadev commented Mar 5, 2023

Perhaps an alternative is to create functions that are neither inflight nor preflight, some flight functions and add the prefix when assigning to a variable

This type of function only becomes executable when the prefix is assigned

something like

// a template function, if no variable receives this template function
// with a prefix (pre/in) the compiler gives an error 
let f = flight(...) { ... }; 
let p = pre f; // create a preflight function
let i = in f; // create a inflight function

@ShaiBer
Copy link
Contributor

ShaiBer commented Mar 5, 2023

@Chriscbr , great doc Can you also add the reasoning and need for phase-independent methods, have you came across any concrete use cases?

Maybe I'm missing something, but I think any project has general purpose utility functions that are used throughout the code and have no business being confined to only one phase. For example: sorting an array with different algorithms

@Chriscbr
Copy link
Contributor Author

Chriscbr commented Mar 6, 2023

Yes, I think @ShaiBer 's answer is correct, the main use case that comes to my mind is utility functions that do not need to be restricted to one phase or another. If I think of any more use cases I'll update the proposal.

BTW, my initial guess is we don't need this feature for beta, or at least we should dogfood more before considering it.

@marciocadev that's a very interesting idea! From an implementation angle, I think that could be a good option if we want to simplify modeling phase-independent functions in the compiler (perhaps we would only need our Phase enum to have two choices instead of three).

But I'm a little cautious about templating based solutions / not sure I understand the benefits. In your example, suppose I use a preflight function inside f (this means it can only be used in preflight). Would I only get a compiler error once I write let i = in f? Or would I get an error on the let f = line?

@marciocadev
Copy link
Collaborator

marciocadev commented Mar 6, 2023

I was thinking in gives an error if f is declare and never used to create a inflight or preflight function

Some errors

bring cloud;

let f = flight ( ... ) { ... } // ERROR, something like f variable never used
bring cloud;

let f = flight ( ... ) { ... }

new cloud.Function(pre f); // ERROR, can not pass a preflight to cloud.Function

This works

bring cloud;

let f = flight ( ... ) { ... }

new cloud.Function(in f); // WORKS

Somehow, I think of a preflight function as a function that will only be performed during the stack deploy. It is something that can be executed during the deploy phase, either before building the stack, during its construction, or after it has been built.

bring cloud;

let f = flight ( ... ) { ... } // verify some data
let p = pre f;
if (p.invoke() == false) { // verify before create the cloud.Function (preflight)
   ...
}

let i = in f;

new cloud.Function(inflight(event: str): str => {
   ...
  if (i.invoke() == true) { // verify inside cloud.Function (inflight)
     ...
  }
  ...
}

if (p.invoke() == true) { // verify after create the cloud.Function (preflight)
   ...
}

or the same without create new variables

bring cloud;

let f = flight ( ... ) { ... } // verify some data
if ((pre f).invoke() == false) { // verify before create the cloud.Function (preflight)
   ...
}

new cloud.Function(inflight(event: str): str => {
   ...
  if ((in f).invoke() == true) { // verify inside cloud.Function (inflight)
     ...
  }
  ...
}

if ((pre f).invoke() == true) { // verify after create the cloud.Function (preflight)
   ...
}

@Chriscbr
Copy link
Contributor Author

Another naming suggestion from @MarkMcCulloh is "unphased"

@github-actions
Copy link

Hi,

This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days.
If you need help with the PR, do not hesitate to reach out in the winglang community slack at winglang.slack.com.
Feel free to re-open this PR when it is still relevant and ready to be worked on again.
Thanks!

@github-actions github-actions bot added the Stale label Apr 18, 2023
@Chriscbr Chriscbr removed the Stale label Apr 18, 2023
@github-actions
Copy link

Hi,

This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days.
If you need help with the PR, do not hesitate to reach out in the winglang community slack at winglang.slack.com.
Feel free to re-open this PR when it is still relevant and ready to be worked on again.
Thanks!

@github-actions github-actions bot added the Stale label May 11, 2023
@Chriscbr Chriscbr removed the Stale label May 14, 2023
@Chriscbr Chriscbr marked this pull request as draft May 26, 2023 18:45
@github-actions
Copy link

Hi,

This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days.
If you need help with the PR, do not hesitate to reach out in the winglang community slack at winglang.slack.com.
Feel free to re-open this PR when it is still relevant and ready to be worked on again.
Thanks!

@github-actions github-actions bot added the Stale label Jun 17, 2023
@Chriscbr Chriscbr removed the Stale label Jun 20, 2023
@skyrpex
Copy link
Contributor

skyrpex commented Aug 29, 2023

I'd expect Wing to infer the phase of the function most of the time, and also cast a function to inflight if necessary. In the same way that TypeScript can infer that the following function returns a number:

export const sum = (a: number, b: number) => {
  return a + b;
};

Regarding Chris' point:

When using a Wing library, consumers should be able to rely on the fact that the method always works in that phase in the future, even if it the implementation changes

I like Mark's unphased keyword to enforce that a function works in both preflight and inflight phases.

Here's how I'd like it to work in different scenarios:

Compiler infers that the function is inflight

bring cloud;

new cloud.Function(() => {
  return "hello";
});

Compiler can cast phase-independent functions to inflight (if they don't interact with resources nor capture mutable state)

bring cloud;

let sum = (a: num, b: num) => {
  return a + b;
}

log(sum(1, 1));

new cloud.Function(() => {
  return sum(2, 2);
});

Compiler infers the function is inflight so it has access to bucket's inflight API

bring cloud;

let bucket = new cloud.Bucket();

new cloud.Function(() => {
  bucket.put("hello", "world");
});

Compiler infers that "put" is preflight because it accesses the bucket

bring cloud;

let bucket = new cloud.Bucket();

let putFile = (key: str, value: str) => {
  bucket.put(key, value);
  // ^ Error: "bucket.put" is an inflight method but "putFile" is a preflight method. Try adding the inflight keyword to "putFile".
};

Compiler infers that "append" is preflight because it captures mutable state

bring cloud;

let mut state = "";

let append = (value: str) => {
  state = "${state} ${value}";
};

new cloud.Function(() => {
  append("hello");
  // ^ Error: Can't use "append" because it's a preflight function that captures mutable state.
});

@Chriscbr
Copy link
Contributor Author

@skyrpex Feel free to add a +1 to the issue about inferring function phases here: #3755

The intuition in your examples is correct - I think unphased functions should be possible to use both in places that expect preflight functions and places that expect inflight functions.

@Chriscbr Chriscbr mentioned this pull request Oct 6, 2023
5 tasks
@exoego exoego mentioned this pull request Oct 8, 2023
5 tasks
Copy link

github-actions bot commented May 5, 2024

Hi,

This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days.
If you need help with the PR, do not hesitate to reach out in the winglang community slack at winglang.slack.com.
Feel free to re-open this PR when it is still relevant and ready to be worked on again.
Thanks!

@Chriscbr Chriscbr changed the title rfc: phase-independent functions with unphasedkeyword rfc: unphased functions Jul 1, 2024
@Chriscbr Chriscbr requested a review from a team July 1, 2024 20:19
@Chriscbr Chriscbr added the 🚧 pr/do-not-merge PRs with this label will not be automatically merged by mergify. label Jul 1, 2024
@Chriscbr Chriscbr marked this pull request as ready for review July 1, 2024 21:00
@Chriscbr Chriscbr removed the 🚧 pr/do-not-merge PRs with this label will not be automatically merged by mergify. label Jul 1, 2024
Using this modifier means that the function can be called from either preflight or inflight contexts.

```TS
let odd_numbers = unphased (arr: Array<num>): Array<num> => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the suggestion but I prefer inflight? instead of introducing another keyword to the language.

I think it reads nicely:

let foo = inflight? () => {
  return 34;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With just inflight, preflight, unphased perspective I think it reads nice, but my concern here is the first thing I think of when I see inflight? is an optional inflight.

(Thinking outloud)
The RFC mentions:

However, a preflight or inflight function cannot be passed to a function that expects an unphased function.

How would an expected unphased function look in a method signature if we use inflight?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried there could be a confusion or mental overloading with "?" since "foo?" looks a lot like an optional type. (It's also handy that "unphased" is fewer characters). But I'm not sure - let's keep the option open, it's probably the easiest part of the design to change 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that “optional inflight” is actually conveying the idea of “unphased” pretty well.

It implies that this is a preflight function that can also be used inflight.

We should be very careful to add additional keywords to the language. Every new keyword is a huge cognitive overload.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that thinking of the function as "optionally inflight" means it could be inflight or preflight.

However, we define optionality in the language as potentially having a "lack of value" so T? is [T || nil], which makes inflight? being [inflight || preflight] weird in my opinion.

docs/contributing/999-rfcs/2023-06-12-language-spec.md Outdated Show resolved Hide resolved
this.name = "my-bucket";
}

unphased object_url(key: str): str {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big fan of the keyword approach over the previous ?, it feels very clear and is consistent with existing keyword modifiers.

docs/contributing/999-rfcs/2023-06-12-language-spec.md Outdated Show resolved Hide resolved
mergify bot pushed a commit that referenced this pull request Jul 11, 2024
First step towards implementing the `unphased` functions RFC (#1711)

## Checklist

- [ ] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [ ] 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)*.
@Chriscbr
Copy link
Contributor Author

Chriscbr commented Jul 12, 2024

I've added an addendum with a proposed mechanism for how unphased extern functions can be implemented. It's not a high priority to support this immediately, but this will help us build a path forward for rewriting various parts of the util and fs modules in Wing.

boyney123 pushed a commit that referenced this pull request Jul 15, 2024
First step towards implementing the `unphased` functions RFC (#1711)

## Checklist

- [ ] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [ ] 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)*.
@skyrpex
Copy link
Contributor

skyrpex commented Jul 26, 2024

Random thought: unphased vs universal?

@@ -1983,6 +2086,36 @@ matching name (without any case conversion).

Extern methods do not support access to class's members through `this`, so they must be declared `static`.

If an extern function is declared as `unphased`, a preflight and inflight implementation must be provided, by providing two separate functions in the JavaScript module.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could also still accept a function, if both implementations are the same. I'm just trying to avoid clutter.

So it could be:

exports.randomHex = async function (length) {
    const buffer = Buffer.alloc(length);
    await randomFillAsync(buffer);
    return buffer.toString("hex");
}

but also:

exports.randomHex = {
  preflight: function (length) {
    const buffer = Buffer.alloc(length);
    crypto.randomFillSync(buffer);
    return buffer.toString("hex");
  },
  inflight: async function (length) {
    const buffer = Buffer.alloc(length);
    await randomFillAsync(buffer);
    return buffer.toString("hex");
  }

A simple check could be used to detect both cases: exports.randomHex instanceof Function ? "single implementation" : "dual implementation".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, this assumes externs can only be functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds ok to me as an extra enhancement (I probably wouldn't implement it for the MVP but maybe later).

Note: if you only provide a single function, it will need to be sync (not async), otherwise it can't be safely called in preflight.

Copy link
Contributor Author

@Chriscbr Chriscbr Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also FWIW possible to reuse a single implementation twice in the JavaScript (so I'm not sure it's worth slowing down performance by making it Wing's responsibility to check if externs are instanceof Function each time an extern is called).

function randomHex(length) {
  const buffer = Buffer.alloc(length);
  await randomFillAsync(buffer);
  return buffer.toString("hex");
}
exports.randomHex = { preflight: randomHex, inflight: randomHex };

@Chriscbr Chriscbr self-assigned this Aug 1, 2024
@eladb eladb closed this Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants