Skip to content

[vm/ffi] fromFunction and asFunction drop type argument and return dynamic #37464

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

Closed
dcharkes opened this issue Jul 8, 2019 · 5 comments
Closed
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Jul 8, 2019

No description provided.

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Jul 8, 2019
@sjindel-google
Copy link
Contributor

Can you please clarify the title/description of the task?

I'm not sure if you're identifying a bug or feature.

@dcharkes
Copy link
Contributor Author

dcharkes commented Aug 23, 2019

If I remember correctly, we created this task during a discussion about the API. I think the goal was to remove locations in the source code which are invariant/covariant/contravariant. By removing the type argument from asFunction() it becomes invariant by construction (but dynamically typed).

For fromFunction I don't remember what we discussed.

If there is a bug in variance checking for fromFunction and asFunction then this should be classified as bug. Though a bug could also be fixed without changing the API.

Edit: see #37385

@sjindel-google
Copy link
Contributor

Ah yes, #37853 is the bug and this is the API cleanup.

@dcharkes
Copy link
Contributor Author

This is a trade-off between verbosity and static checking. Removing the type argument to asFunction and lookupFunction makes it less verbose. However, it also makes it so that type errors are only caught when assigning the returned closure to a variable with some function signature.

/cc @mkustermann what do you think?

@sjindel-google
Copy link
Contributor

We agreed offline to leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

2 participants