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

Execute foreign function and check autoscoped constructor result #10187

Merged
merged 9 commits into from
Jun 6, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jun 5, 2024

Pull Request Description

Fixes #10151 and also fixes #10180 and fixes #10186.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Java,
  • Unit tests have been written where possible.
  • Obsolete unit test was rewritten by @4e6

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 5, 2024
@JaroslavTulach JaroslavTulach self-assigned this Jun 5, 2024
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Enso tests look good

@JaroslavTulach JaroslavTulach changed the title Execute foreign function Execute foreign function and check autoscoped constructor result Jun 6, 2024
msg . should_contain "Expected `..Value` to be Foo"
msg . should_contain "got Foo.Value["
msg . should_contain "foo=_"
msg . should_contain "Try to apply foo argument"
Copy link
Member Author

@JaroslavTulach JaroslavTulach Jun 6, 2024

Choose a reason for hiding this comment

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

Running a.enso:

from Standard.Base import all

type Foo
    Value a

main =
    v = ..Value
    f = v:Foo
    f

yields

Type error: Expected `..Value` to be Foo, but got Foo.Value[a.enso:4:5-11] a=_.
Try to apply a argument.
        at <enso> a.main(a.enso:8:9-9)

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

LGTM. Minor: consider adding few tests for edge cases invoking polyglot functions.

@@ -34,6 +34,14 @@ add_specs suite_builder = suite_builder.group "Polyglot" group_builder->
group_builder.specify "access Integer field from Polyglot object" <|
js_meaning.meaning . should_equal 42

group_builder.specify "Execute JavaScript function" <|
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to also see what happens if I try to specify more or less arguments to the function. Can you do, e.g.,

curried = js_plus 3
curried 5 . should_equal 8

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Invoking:

curried = js_plus 3

main =
    curried

foreign js js_plus = """
    return (a, b) => a + b;

yields NaN as 3 + undefined is NaN.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect it to behave the same way as if js_plus was an enso function. I.e. main should return a function

Copy link
Member Author

@JaroslavTulach JaroslavTulach Jun 6, 2024

Choose a reason for hiding this comment

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

Tested in fe14b8d

I'd expect it to behave the same way as if js_plus was an enso function

You might expect it, but there is no chance to achieve such behavior. JavaScript functions don't have a fixed arity. You can invoke them with any number of arguments. If you want that to be changed, then you need to beg for a change of the ECMAScript specification ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, I didn't think about that

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Jun 6, 2024
@mergify mergify bot merged commit 396d70d into develop Jun 6, 2024
37 checks passed
@mergify mergify bot deleted the wip/jtulach/ForeignFunction10151 branch June 6, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
7 participants