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

@directives in functions #6756

Merged
merged 3 commits into from
May 16, 2024
Merged

@directives in functions #6756

merged 3 commits into from
May 16, 2024

Conversation

zth
Copy link
Collaborator

@zth zth commented May 15, 2024

This is a base implementation for allowing @directive on functions, which will emit a directive at the top of the function body. This is the equivalent of the top level @@directive we currently have. Directives are needed for React server actions, the new React compiler, and so on.

let testFnWithDirective = @directive("'use memo'") (name: string) => "Hello " ++ name

Emits:

// Generated by ReScript, PLEASE EDIT WITH CARE
'use strict';


function testFnWithDirective(name) {
  'use memo';
  return "Hello " + name;
}

exports.testFnWithDirective = testFnWithDirective;
/* No side effect */

@cristianoc see any immediate issues here? The only thing I can think of is that these functions shouldn't be inlined/optimized away, because the directive needs to be preserved. Not sure if that's already the case here or if more things need to be adjusted for that to not happen.

@zth zth requested a review from cristianoc May 15, 2024 19:44
@cristianoc
Copy link
Collaborator

cristianoc commented May 16, 2024

Looks good to me.
For inlining, one could look into js_pass_tailcall_inline.ml and change this line:

                           Fun {is_method=false; params; body; env; async=false};

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Looks good, left a comment about how to prevent inlining.
Btw is it true that one wants to prevent inlining for each possible directive?

@zth zth requested a review from cristianoc May 16, 2024 09:42
@zth zth merged commit b1d5a43 into master May 16, 2024
14 checks passed
@zth zth deleted the directive-for-functions branch May 16, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants