-
Notifications
You must be signed in to change notification settings - Fork 850
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
Adding LambdaAccessorSlot #1577
Changes from 1 commit
4a27e8e
e0af91a
d5cafc7
c4789fc
e099931
b9c528b
0d53988
7369f3f
323396f
a48d1c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
package org.mozilla.javascript; | ||
|
||
import java.util.IdentityHashMap; | ||
import java.util.function.BiConsumer; | ||
import java.util.function.Function; | ||
|
||
/** | ||
* A specialized property accessor using lambda functions, similar to {@link LambdaSlot}, | ||
* but allows defining properties with getter and setter lambdas that require access to the owner object ('this'). | ||
* This enables the implementation of properties that can access instance fields of the owner. | ||
* | ||
* Unlike {@link LambdaSlot}, Lambda functions used to define getter and setter logic require the owner's | ||
* `Scriptable` object as one of the parameters. | ||
* This is particularly useful for implementing properties that behave like standard JavaScript properties, | ||
* but are implemented with native functionality without the need for reflection. | ||
*/ | ||
public class OwnerAwareLambdaSlot extends Slot { | ||
private transient Function<Scriptable, Object> getter; | ||
private transient BiConsumer<Scriptable, Object> setter; | ||
OwnerAwareLambdaSlot(Object name, int index) { | ||
super(name, index, 0); | ||
} | ||
|
||
OwnerAwareLambdaSlot(Slot oldSlot) { | ||
super(oldSlot); | ||
} | ||
|
||
@Override | ||
boolean isValueSlot() { | ||
return false; | ||
} | ||
|
||
|
||
@Override | ||
ScriptableObject getPropertyDescriptor(Context cx, Scriptable scope) { | ||
ScriptableObject desc = (ScriptableObject) cx.newObject(scope); | ||
if (getter != null) { | ||
desc.defineProperty("get", | ||
new LambdaFunction(scope, "get " + super.name, 0, (cx1, scope1, thisObj, args) -> | ||
getter.apply(thisObj)), ScriptableObject.EMPTY); | ||
} | ||
|
||
if (setter != null) { | ||
desc.defineProperty("set", | ||
new LambdaFunction(scope, "set " + super.name, 1, (cx1, scope1, thisObj, args) -> | ||
{ | ||
setter.accept(thisObj, args[0]); | ||
return Undefined.instance; | ||
}), ScriptableObject.EMPTY); | ||
} | ||
desc.setCommonDescriptorProperties(getAttributes(), false); | ||
return desc; | ||
} | ||
|
||
@Override | ||
public boolean setValue(Object value, Scriptable scope, Scriptable owner, boolean isStrict) { | ||
if (setter != null) { | ||
setter.accept(owner, value); | ||
return true; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 🤣) |
||
if (isStrict) { | ||
// in strict mode | ||
throw ScriptRuntime.typeErrorById("msg.modify.readonly", name); | ||
} else { | ||
super.setValue(value, scope, owner, false); | ||
} | ||
|
||
return true; | ||
} | ||
|
||
@Override | ||
public Object getValue(Scriptable owner) { | ||
if (getter != null) { | ||
return getter.apply(owner); | ||
} | ||
Object v = super.getValue(owner); | ||
return v == null ? Undefined.instance : v; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is also different to LambdaSlot |
||
} | ||
|
||
public void setGetter(Function<Scriptable, Object> getter) { | ||
this.getter = getter; | ||
} | ||
|
||
public void setSetter(BiConsumer<Scriptable, Object> setter) { | ||
this.setter = setter; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
in LambdaSlot with your suggestion
I would need to adjust the existing defineProperty this way.
What I want to say: Your modified
OwnerAwareLambdaSlot
can cover the whole functionality ofLambdaSlot
We only require to convert
Supplier<Object>
into aFunction<Scriptable, Object>
and respectivelyConsumer<Object>
into aBiConsumer<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:This would only require 2-3 additional null checks, which is IMHO an acceptable tradeoff of additional complexity and CPU cycles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.