Adding LambdaAccessorSlot #1577

nabacg · 2024-08-22T08:51:16Z

First time contributing, so let me know if I missed something important.

I've noticed the new LambdaConstructor based pattern of registering scriptables is being used more and more (NativePromise or NativeMath) and I find it's very clear and elegant to define javascript object methods with java's Callable via Lambda Functions, but I came across the limitation where if you want to define a property where it's Getter and/or Setter logic needs to access js object instance state ( needs a reference to this ) there isn't really a good way to do this on a LambdaConstructor object. NativePromise intentionally doesn't have any getter / setter properties, so it didn't run into this problem. Yes, it's possible it use ScriptableObject.defineProperty, but it requires Method parameters which forces reflection and wouldn't it be so much nicer if we could define properties using Lambda functions like
Function<Scriptable, Object> and BiConsumer<Scriptable, Object> and write code like this:

constructor.definePrototypeProperty("status",
                        (thisObj) -> self(thisObj).getStatus(),
                        (thisObj, value) -> self(thisObj).setStatus(value), 
DONTENUM);

This PR is attempting to solve this problem by adding new type of Slot called OwnerAwareLambdaSlot, inspired by existing LambdaSlot. It allows defining getter / setter properties using Lambda functions that accept Scriptable owner object ('this') as one of parameters, thus allowing to implement properties that require access to instance fields etc.

While I don't have specific use case for it in the code base (but if you can think of one, I'm happy to part take), I've built it for some internal use cases I sadly can't share, but I hope the new slot itself and the unit tests that demonstrate how to use it is also useful. Reading comments on some of the recent PRs makes me thing that more people would find it useful and hopefully would facilitate more refactoring into this new pattern:

…ning getter / setter properties using Lambda functions that accept owner object ('this') as one of parameters, thus allowing to implement properties that require access to instance fields etc.

tests/src/test/java/org/mozilla/javascript/tests/OwnerAwareLambdaSlotTest.java

rPraml · 2024-08-22T09:42:23Z

rhino/src/main/java/org/mozilla/javascript/OwnerAwareLambdaSlot.java

+            setter.accept(owner, value);
+            return true;
+        }
+


This code is slightly different to https://github.com/mozilla/rhino/blob/master/rhino/src/main/java/org/mozilla/javascript/LambdaSlot.java#L52

can this be refactored to have the same code flow like in LambdaSlot?

Well I was trying to improve it slightly be implementing the can't set getter only property error (in strict mode only): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Getter_only

if you don't like it, sure I can align them.

if you don't like it...

No, I didn't dislike it. I was also wondering, why LambdaSlot behaves different now.

As far as I understand, your intention in this PR is, to define also properties, where you can access 'thisObj' (very similar to the existing code, that uses the LambdaSlot) right?

IMHO both code paths should behave the same. So if OwnerAwareLambda slot throws an exception in strict mode, LambdaSlot should do the same.

Or is there any difference between the two cases, that I didn't get, yet?

(Please note: I'm no maintainer, only contributor, so my comments might be totally wrong 🤣)

rPraml · 2024-08-22T09:43:28Z

rhino/src/main/java/org/mozilla/javascript/OwnerAwareLambdaSlot.java

+            return getter.apply(owner);
+        }
+        Object v = super.getValue(owner);
+        return v == null ? Undefined.instance : v;


this is also different to LambdaSlot

rPraml · 2024-08-22T10:01:46Z

rhino/src/main/java/org/mozilla/javascript/OwnerAwareLambdaSlot.java

+import java.util.function.Function;
+
+/**
+ * A specialized property accessor using lambda functions, similar to {@link LambdaSlot},


As far as I understand, the OwnerAwareLambdaSlot is very similar to LambdaSlot. (but code flow is a bit different. see comments)

I made a quick and dirty test, If I replace

transient Supplier<Object> getter; transient Consumer<Object> setter;

in LambdaSlot with your suggestion

transient Function<Scriptable, Object> getter; transient BiConsumer<Scriptable, Object> setter;

I would need to adjust the existing defineProperty this way.

public void defineProperty( String name, Supplier<Object> getter, Consumer<Object> setter, int attributes) { LambdaSlot slot = slotMap.compute(name, 0, ScriptableObject::ensureLambdaSlot); slot.setAttributes(attributes); slot.getter = (start) -> getter.get(); slot.setter = (start, value) -> setter.accept(value); }

What I want to say: Your modified OwnerAwareLambdaSlot can cover the whole functionality of LambdaSlot

We only require to convert Supplier<Object> into a Function<Scriptable, Object> and respectively Consumer<Object> into a BiConsumer<Scriptable, Object>

The question is, what is more important: Having two classes with nearly the same logic, and save some CPU cycles not compiling an additional lambda or having only one class which gurantees, that code does not diverge. (which already happen)

(!) I have no clear oppinion about this, so maybe wait, if there is an additional feedback

Maybe there is a third option. We could extend the existing LambdaSlot this way:

transient Supplier<Object> getter; transient Function<Scriptable, Object> ownerAwareGetter; transient Consumer<Object> setter; transient BiConsumer<Scriptable, Object> ownerAwareSetter;

This would only require 2-3 additional null checks, which is IMHO an acceptable tradeoff of additional complexity and CPU cycles

Perfectly reasonable point to combine both into single class, although I also don't have strong opinion either way. I wasn't sure what the original intention behind LambdaSlot was, so it felt simpler to add another class.

I would welcome more feedback.

…bdaSlotTest.java Co-authored-by: Roland Praml <[email protected]>

rPraml · 2024-08-22T11:44:35Z

tests/src/test/java/org/mozilla/javascript/tests/OwnerAwareLambdaSlotTest.java

@@ -74,7 +74,7 @@ public void testCanUpdateValueUsingSetter() {
        Object setResult = cx.evaluateString(scope, "s.status = 'DONE';", "source", 1, null);

        Object newStatus = cx.evaluateString(scope, "s.status", "source", 1, null);
-        assertEquals( "NewStatus: DONE", newStatus);
+        assertEquals("NewStatus: DONE", newStatus);


Hello @nabacg - this was only one change I noticed, that the code is not correctly indented/aligned.

There are several more, if you take a look at the last build: https://github.com/mozilla/rhino/actions/runs/10504907544/job/29103912052

You will need to fix this violations first with ./gradlew spotlessApply (You should not the "Format code" feature of your IDE)

If you need help, let me know

I'm sure I've run ./gradlew spotlessApply before raising the PR, but now I see that I was running Java 17 and spotlessApply seems to do nothing? I switched to Java 11 and that made the difference d5cafc7.

I guess this is mentioned here, rookie mistake!

This also happens to me in every new console window, when I've forgotten to change JAVA_HOME 🤣

gbrail · 2024-08-23T05:16:30Z

This is a very complete PR! I was working on lambda stuff and had a need for the exact same thing, so I'm glad you did it! Now we need to talk about the details:

Today, there are a few varieties of ScriptableObject.defineProperty, and a few Slot subclasses.

Just takes a raw value and sets it directly, so that's not interesting here ;-). Uses a regular Slot.
One (actually "defineOwnProperties") takes a Scriptable descriptor and does just what it says in the spec, looking at attributes and supporting attaching Function objects to get and set the value. May use a Slot or AccessorSlot, which calls the getter and setter as if they were JavaScript functions.
One takes a Java class and can use reflection to create wrappers for various functions by calling the next method...
One takes a pair of Method objects and uses Java reflection to create wrappers, and creates an AccessorSlot
One takes a Getter and a Setter interface and is optimized for lambdas, and creates a LambdaSlot
One takes a single Callable and I'm not sure where we use it, even though I think I wrote it!

#6 behaves differently from #2 through #5 in that the LambdaSlot is invisible to the end user -- the property value is get and set as if it were just stored natively by Rhino. #2 to #5, on the other hand, allow the user to introspect the property and get at the JavaScript functions to "get" and "set" the value.

I'm saying this because what it looks like you did here is create something that looks to the user like the AccessorSlot was used, but it's more optimized for lambda functions. However, it's different from the existing LambdaSlot in that the resulting descriptor looks like the setter and getter were implemented as Java functions and not totally in a native way.

Also, I see that in the descriptor you're actually creating a new LambdaFunction on the fly every time -- I worry not only about performance but if that will pass the sniff test for all of test262, since sometimes we check that the same function is returned every time from a property, and here we generate a new function every time "getOwnPropertyDescriptor" is called.

I'd really like you to finish this (and I want to use it for stuff that I want to do), so can I suggest a few things:

Create the getter and setter "LambdaFunction" instance when the property is set -- once they're visible in in the property descriptor, people might call them, so we should at least only create them once, and then possibly even invoke that function through the Callable interface on actual get and set calls to make sure that we did it right. (I'm not totally sold on the last part, but I am on the first part.)
Instead of calling it "OwnerAwareLambdaSlot," we should instead call it something like "LambdaAccessorSlot".
If you look at the implementation of "defineOwnProperty," you'll see that it does additional validation, like making sure that the slot is writable or configurable or whatever. I know for now we'll only use this to define properties on prototypes, but in the interest of long-term success we might want to include some of that logic too.

Finally, I think that we ALSO will eventually need a version of LambdaSlot that works exactly like the current one, but passes "self." The interface would look just like this one but it would instead not show up to the user as an accessor property. I think that we could do that by changing the things that LambdaSlot calls and adding a wrapper for backward compatibility.

Thanks for reading a long comment!

p-bakker · 2024-08-23T05:36:34Z

Reading the comment of @gbrail above, I wonder if it is in any way related to #1549 which is about some of the JavaScript APIs related to propertyDescriptors not working on Java objects

…etPropertyDescriptor only creates single instance of get/set LambdaFunctions, instead of instance per call

nabacg · 2024-08-23T08:09:31Z

Thanks for a review @p-bakker, @gbrail and for comprehensive explanation of all the slot varieties! I just pushed changes for the easy stuff, renaming the slot and making sure I don't re-create LambdaFunction instances for each getOwnPropertyDescriptor call. I'll need to think how I could incorporate validation like checkPropertyChange into this, hopefully will have some time to work on it this weekend.

@gbrail regarding the topic of combining LambdaSlot with LambdaAccessorSlot, it sounds like you're saying they should stay as separate (due to one being hidden from introspection, etc) but LambdaSlot might need to evolve to support self as well. Did I get this right?

nabacg · 2024-08-23T08:12:51Z

Finally, I'm happy to continue working on everything listed above, but let's agree it won't all be solved in a single PR.. :)

p-bakker · 2024-08-23T14:32:07Z

When I referred to #1549, I wasn't very clear on how I see it potentially being related :-)

I meant to just keep in the back of our minds the issues reported in #1549 and make sure whatever we do with slots either improves the issues reported in #1549 or otherwise at least won't make it harder to fix those issues in the future

gbrail · 2024-08-23T23:00:21Z

Yes -- I think that "LambdaSlot" and "LambdaAccessorSlot" should stay separate. However, in the future I can imagine that we might need a version of LambdaSlot that passes the "this" value, but since we don't have that use case today, why don't we "YAGNI" and wait until we do -- whereas I know that we need this PR so that we can finish the implementation of Symbol. Thanks!

… PropertyDescriptor sets writable, enumerable and configurable attributes, plus calling validation on those when creating new LambdaAccessorSlot properties

nabacg · 2024-08-25T17:53:48Z

Ok, I pushed a few more commits, going through the short list @gbrail provided:

Create the getter and setter "LambdaFunction" instance when the property is set -- once they're visible in in the property descriptor, people might call them, so we should at least only create them once, and then possibly even invoke that function through the Callable interface on actual get and set calls to make sure that we did it right. (I'm not totally sold on the last part, but I am on the first part.)

I didn't go as far as the last part, i.e. accessing property via Callable interface, but everything else is done.

Instead of calling it "OwnerAwareLambdaSlot," we should instead call it something like "LambdaAccessorSlot".

Renamed.

If you look at the implementation of "defineOwnProperty," you'll see that it does additional validation, like making sure that the slot is writable or configurable or whatever. I know for now we'll only use this to define properties on prototypes, but in the interest of long-term success we might want to include some of that logic too.

Changed new defineProperty to call both

in similar fashion as defineOwnProperty does and I added couple of tests to assert we go through those validations.

I tried to bring LambdaAccessorSlot to look and behave similar to AccessorSlot, like throwing the same errors and setting those configurable, enumerable, etc properties here.

Overall the feedback that LambdaAccessorSlot should behave more like AccessorSlot not LambdaSlot was super helpful.

gbrail

Looking good -- just an unused function crept in there. Thanks!

gbrail · 2024-08-27T03:17:51Z

rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java

@@ -2695,6 +2781,17 @@ private static LambdaSlot ensureLambdaSlot(Object name, int index, Slot existing
        }
    }

+    private static LambdaAccessorSlot ensureLambdaAccessorSlot(


Looks like we're not using this method any more... FWIW, I was using this pattern as a simple way to switch slot classes in a clean way, and then doing the calculation after that, so the way I did this in other methods is a bit different from what you did, but it's all good.

But this is unused code so we should delete it!

Thanks, I missed that one. It's gone.

I was using this pattern as a simple way to switch slot classes in a clean way, and then doing the calculation after that, so the way I did this in other methods is a bit different from what you did, but it's all good.

The version ofensureLambdaAccessorSlot I ended up using is closer to this lambda function in defineOwnProperty. It's not just (re)creating slot classes, but also doing validation, etc. If you think it's doing to much in a single function or could be named better, I'm happy to refactor this too @gbrail .

Although maybe in next PR? :)

gbrail · 2024-08-28T04:48:49Z

rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java

+     * need to have access to target object instance, this allows for defining properties on
+     * LambdaConstructor prototype providing getter and setter logic with java instance methods. If
+     * a property with the same name already exists, then it will be replaced. This property will
+     * appear to the JavaScript user exactly like any other property -- unlike Function properties


I'm sorry I keep finding things, but now this comment is out of date-- as you've defined it, and unlike the other "defineProperty" that takes Getter and a Setter, this property will not look like a normal value, but like a descriptor with a getter and setter, just as if they had been defined in JavaScript using Object.defineOwnProperty. Could you please update? I think this will be the last thing!

Of course, it's not great to leave outdated comments! Especially in this case where it can already be pretty confusing which defineProperty should be used. I should have noticed this myself, thanks for being patient with me!

p-bakker · 2024-08-28T16:16:29Z

rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java

+        boolean es6 = cx.getLanguageVersion() >= Context.VERSION_ES6;
+        if (es6) {
+            if (getterFunction == null && setterFunction == null) {
+                desc.defineProperty(


Not 100% sure, but doesn't the pd need a 'value' property as well if it won't have getter?

Good question. I pretty much followed how AccessorSlot implements this.

Having said that, trying below

> const o = {}; undefined >Object.defineProperty(o, "b", { enumerable: true, configurable: true }); {b: undefined} > Object.getOwnPropertyDescriptor(o, "b") {value: undefined, writable: false, enumerable: true, configurable: true}

in both Chrome and nodejs does return value: undefined so perhaps we should add it?

p-bakker · 2024-08-28T18:22:39Z

Am pretty sure it ought to behave like you've seen in Chrome

Rhino unfortunately has a bunch of issues In the area of property descriptors, but if we can prevent introducing a new one... 🙂

nabacg · 2024-08-28T20:01:29Z

Am pretty sure it ought to behave like you've seen in Chrome

Rhino unfortunately has a bunch of issues In the area of property descriptors, but if we can prevent introducing a new one... 🙂

If I'm going to make that change, it would probably make sense to change it in both LambdaAccessorSlot and AccessorSlot. I would appreciate @gbrail's thoughts, since it was his original suggestion that both classes should be aligned.

gbrail · 2024-08-28T20:18:42Z

FWIW I think that changing how property descriptors in general is out of the scope of this PR and if we want to change it, we should do it in another PR.

…

On Wed, Aug 28, 2024 at 1:02 PM Grzegorz Caban ***@***.***> wrote: @nabacg <https://github.com/nabacg> requested your review on: #1577 <#1577> Adding LambdaAccessorSlot. — Reply to this email directly, view it on GitHub <#1577 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I24PACHL7F376PPYLRLZTYUFBAVCNFSM6AAAAABM5VFKXCVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGA2TEMBXHE4DSOI> . You are receiving this because your review was requested.Message ID: ***@***.***>

p-bakker · 2024-08-29T22:02:52Z

@gbrail you mean not fix the bug in the new slot impl. that got copied over from another slot implementation?

Or that this PR shouldn't be extended to fix the other pd implementation issues?

gbrail · 2024-08-30T03:42:51Z

I think it's fine if this PR extends the pattern that's already in place for the existing "AccessorSlot," and that we merge this and track any inconsistencies that we think there are in the existing implementation, rather than have two ways to do the same basic thing that produce different results.

Also someone will have to explain to me the difference between having a property present but with a value of "undefined" and a value that is not present, but it's JavaScript so I'm sure it's in there somewhere!

nabacg · 2024-08-30T08:47:33Z

I pushed one more commit updating tests to Utils.runWithAllOptimizationLevels as suggested by @rPraml in other PRs. Is there anything else outstanding for this PR?

p-bakker · 2024-08-30T11:51:48Z

Also someone will have to explain to me the difference between having a property present but with a value of "undefined" and a value that is not present, but it's JavaScript so I'm sure it's in there somewhere!

somePDInstance.value will return undefined in both cases, but these will display different behavior:

somePDInstance.hasOwnProperty/Object.hasOwn: false vs. true
Object.getPropertyDescriptor: undefined vs. Object
Object.getPropertyDescriptors/Object.keys/Object.entries: missing entry for value property PD vs including a PD for the value property
forOf/forIn loops won't include the missing value property

I'm sure I missed a few scenarios :-)

p-bakker · 2024-08-30T11:53:53Z

@nabacg Could you make a follow-up case regarding the missing value property with all relevant info you have and tagit with the PropertyDescriptors label?

nabacg · 2024-08-30T18:37:23Z

Issue #1601, I don't think I have permissions to assign a label.

p-bakker · 2024-08-30T18:53:02Z

I don't think I have permissions to assign a label.

Ah, didn't know that was restricted, added them now

Thx for this follow-up!

gbrail · 2024-08-31T17:22:46Z

Thanks for everyone's work on this!

adding new type of Slot called OwnerAwareLambdaSlot, that allows defi…

4a27e8e

…ning getter / setter properties using Lambda functions that accept owner object ('this') as one of parameters, thus allowing to implement properties that require access to instance fields etc.

rPraml reviewed Aug 22, 2024

View reviewed changes

Update tests/src/test/java/org/mozilla/javascript/tests/OwnerAwareLam…

e0af91a

…bdaSlotTest.java Co-authored-by: Roland Praml <[email protected]>

rPraml reviewed Aug 22, 2024

View reviewed changes

andreabergia mentioned this pull request Aug 22, 2024

Implement Symbol.prototype.description and refactor Symbol to use LambdaConstructor #1579

Closed

nabacg added 2 commits August 22, 2024 16:48

re-formating with spotlessApply

d5cafc7

getValue to return null, not Undefined

c4789fc

p-bakker mentioned this pull request Aug 22, 2024

Add ES2019 Symbol.prototype.description #1561

Closed

OwnerAwareLambdaSlot -> LambdaAccessorSlot rename, plus making sure g…

e099931

…etPropertyDescriptor only creates single instance of get/set LambdaFunctions, instead of instance per call

nabacg added 2 commits August 25, 2024 18:30

Bringing LambdaAccessorSlot more in line with AccessSlot, making sure…

b9c528b

… PropertyDescriptor sets writable, enumerable and configurable attributes, plus calling validation on those when creating new LambdaAccessorSlot properties

spotlessApply

0d53988

nabacg changed the title ~~Adding OwnerAwareLambdaSlot to help define Getter Setter properties on LambdaConstructor~~ Adding LambdaAccessorSlot Aug 25, 2024

gbrail requested changes Aug 27, 2024

View reviewed changes

removing no longer used ensureLambdaAccessorSlot overload

7369f3f

nabacg requested a review from gbrail August 27, 2024 14:46

gbrail reviewed Aug 28, 2024

View reviewed changes

updating outdated comment on defineProperty

323396f

p-bakker reviewed Aug 28, 2024

View reviewed changes

nabacg requested a review from gbrail August 28, 2024 20:02

refactoring tests to use Utils.runWithAllOptimizationLevels

a48d1c0

nabacg mentioned this pull request Aug 30, 2024

AccessorSlot and LambdaAccessorSlot should populate value in Property Descriptor #1601

Open

gbrail approved these changes Aug 31, 2024

View reviewed changes

gbrail merged commit 82416a1 into mozilla:master Aug 31, 2024
3 checks passed

Adding LambdaAccessorSlot #1577

Adding LambdaAccessorSlot #1577

Conversation

nabacg commented Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbrail commented Aug 23, 2024

p-bakker commented Aug 23, 2024

nabacg commented Aug 23, 2024

nabacg commented Aug 23, 2024 • edited Loading

p-bakker commented Aug 23, 2024

gbrail commented Aug 23, 2024

nabacg commented Aug 25, 2024

gbrail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-bakker commented Aug 28, 2024

nabacg commented Aug 28, 2024

gbrail commented Aug 28, 2024 via email

p-bakker commented Aug 29, 2024

gbrail commented Aug 30, 2024

nabacg commented Aug 30, 2024

p-bakker commented Aug 30, 2024

p-bakker commented Aug 30, 2024

nabacg commented Aug 30, 2024

p-bakker commented Aug 30, 2024

gbrail commented Aug 31, 2024

nabacg commented Aug 22, 2024 •

edited

Loading

nabacg commented Aug 23, 2024 •

edited

Loading