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

Function interface vs. Constructable #1376

Open
rbri opened this issue Aug 17, 2023 · 7 comments
Open

Function interface vs. Constructable #1376

rbri opened this issue Aug 17, 2023 · 7 comments

Comments

@rbri
Copy link
Collaborator

rbri commented Aug 17, 2023

While working on the Proxy support i stumbled over some places where we check for instanceof Function and then casting to Function but only calling the construct method on the casted object. This is a problem if we are working with Lambda constructors because they are only implementing Constructable.

What do you think - should we change at least the places where Constructable is sufficient (guess ~5 palces)?

@p-bakker
Copy link
Collaborator

Sounds like it would fix an issue (when using Lambda constructors) without there being any downside, no?

If so, I'd say it makes sense to make such a change

@gbrail
Copy link
Collaborator

gbrail commented Aug 21, 2024 via email

@rbri
Copy link
Collaborator Author

rbri commented Aug 21, 2024

Ok, will wait for your stuff and see...

@rbri
Copy link
Collaborator Author

rbri commented Aug 25, 2024

@gbrail do you think about the Constructable interface as part of this. Just noted that Function extends Constructable. My feeling (at least for the depending lambda stuff) is that we are in trouble here. I guess only lambda constructor has to fulfill this interface and not Lambda function. Correct?

Or you are aware of an different way to figure out if a function is a ctor?

@gbrail
Copy link
Collaborator

gbrail commented Sep 21, 2024

Can you do two things for me, and I'm sorry I haven't focused as much on this so far:

First, background: I added Constructable as a "single-function interface." That way both Constructable and Callable are interfaces that you can pass a lambda function to. That's why we have that.

Second, LambdaConstructor implements both Constructable and Callable, because constructors can be invoked either using "new" or directly. It has a flag that you can set to determine whether both methods just call the constructor, or whether one or the other throws an exception.

Finally, I just merged new support into LambdaConstructor so that you can have the Constructable and Callable be different implementations -- I was trying to make the Date class into a lambda and I needed that, because the two forms of the Date constructor do different things.

So -- given all that, do you need to merge #1635? And if you confirm this, then I'm happy to merge it, and I assume that you need it to complete the proxy implementation?

rbri added a commit to HtmlUnit/rhino-development that referenced this issue Sep 22, 2024
@rbri
Copy link
Collaborator Author

rbri commented Sep 22, 2024

So -- given all that, do you need to merge #1635? And if you confirm this, then I'm happy to merge it, and I assume that you need it to complete the proxy implementation?

No i do not need this for the proxy stuff - there was one place where i had to do the change from Function to Constructable but this is part of the other PR.

The motivation for #1635 was more about using the smallest possible interface - if we only have to call the construct method it is sufficient to check for the Constructable interface.

We can also drop #1635 and wait if we face problems.

@rbri
Copy link
Collaborator Author

rbri commented Sep 22, 2024

@gbrail my real problem with the Reflect/Proxy impl is the implementation of the isConstructor method.

My original plan was to check for the Consturctable interface but this does not work. Maybe you have a better idea.

rbri added a commit to HtmlUnit/rhino-development that referenced this issue Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants