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

Adding LambdaAccessorSlot #1577

Merged
merged 10 commits into from
Aug 31, 2024
129 changes: 129 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
package org.mozilla.javascript;

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.
*
* <p>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 LambdaAccessorSlot extends Slot {
private transient Function<Scriptable, Object> getter;
private transient BiConsumer<Scriptable, Object> setter;
private LambdaFunction getterFunction;
private LambdaFunction setterFunction;

LambdaAccessorSlot(Object name, int index) {
super(name, index, 0);
}

LambdaAccessorSlot(Slot oldSlot) {
super(oldSlot);
}

@Override
boolean isValueSlot() {
return false;
}

@Override
boolean isSetterSlot() {
return true;
}

@Override
ScriptableObject getPropertyDescriptor(Context cx, Scriptable scope) {
ScriptableObject desc = (ScriptableObject) cx.newObject(scope);

int attr = getAttributes();
boolean es6 = cx.getLanguageVersion() >= Context.VERSION_ES6;
if (es6) {
if (getterFunction == null && setterFunction == null) {
desc.defineProperty(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

"writable",
(attr & ScriptableObject.READONLY) == 0,
ScriptableObject.EMPTY);
}
} else {
desc.setCommonDescriptorProperties(
attr, getterFunction == null && setterFunction == null);
}

if (getterFunction != null) {
desc.defineProperty("get", this.getterFunction, ScriptableObject.EMPTY);
}

if (setterFunction != null) {
desc.defineProperty("set", this.setterFunction, ScriptableObject.EMPTY);
} else if (es6) {
desc.defineProperty("set", Undefined.instance, ScriptableObject.EMPTY);
}

if (es6) {
desc.defineProperty(
"enumerable", (attr & ScriptableObject.DONTENUM) == 0, ScriptableObject.EMPTY);
desc.defineProperty(
"configurable",
(attr & ScriptableObject.PERMANENT) == 0,
ScriptableObject.EMPTY);
}
return desc;
}

@Override
public boolean setValue(Object value, Scriptable scope, Scriptable start, boolean isThrow) {
if (setter == null) {
if (getter != null) {
throwNoSetterException(start, value);
return true;
}
} else {
setter.accept(start, value);
return true;
}

return super.setValue(value, start, start, isThrow);
}

@Override
public Object getValue(Scriptable owner) {
if (getter != null) {
return getter.apply(owner);
}
return super.getValue(owner);
}

public void setGetter(Scriptable scope, Function<Scriptable, Object> getter) {
this.getter = getter;
if (getter != null) {
this.getterFunction =
new LambdaFunction(
scope,
"get " + super.name,
0,
(cx1, scope1, thisObj, args) -> getter.apply(thisObj));
}
}

public void setSetter(Scriptable scope, BiConsumer<Scriptable, Object> setter) {
this.setter = setter;
if (setter != null) {
this.setterFunction =
new LambdaFunction(
scope,
"set " + super.name,
1,
(cx1, scope1, thisObj, args) -> {
setter.accept(thisObj, args[0]);
return Undefined.instance;
});
}
}
}
22 changes: 22 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/LambdaConstructor.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

package org.mozilla.javascript;

import java.util.function.BiConsumer;
import java.util.function.Function;

/**
* This class implements a JavaScript function that may be used as a constructor by delegating to an
* interface that can be easily implemented as a lambda. The LambdaFunction class may be used to add
Expand Down Expand Up @@ -120,6 +123,25 @@ public void definePrototypeProperty(Symbol key, Object value, int attributes) {
proto.defineProperty(key, value, attributes);
}

public void definePrototypeProperty(
Context cx,
String name,
java.util.function.Function<Scriptable, Object> getter,
int attributes) {
ScriptableObject proto = getPrototypeScriptable();
proto.defineProperty(cx, name, getter, null, attributes);
}

public void definePrototypeProperty(
Context cx,
String name,
Function<Scriptable, Object> getter,
BiConsumer<Scriptable, Object> setter,
int attributes) {
ScriptableObject proto = getPrototypeScriptable();
proto.defineProperty(cx, name, getter, setter, attributes);
}

/**
* Define a function property directly on the constructor that is implemented under the covers
* by a LambdaFunction.
Expand Down
97 changes: 97 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.function.BiConsumer;
import java.util.function.Consumer;
import java.util.function.Supplier;
import org.mozilla.javascript.ScriptRuntime.StringIdOrIndex;
Expand Down Expand Up @@ -1690,6 +1691,91 @@ public void defineProperty(
slot.setter = setter;
}

/**
* Define a property on this object that is implemented using lambda functions accepting
* Scriptable `this` object as first parameter. Unlike with `defineProperty(String name,
* Supplier<Object> getter, Consumer<Object> setter, int attributes)` where getter and setter
* 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

* and those based on reflection, the property descriptor will only reflect the value as defined
* by this function.
*
* @param name the name of the property
* @param getter a function that given Scriptable `this` returns the value of the property. If
* null, throws typeError
* @param setter a function that Scriptable `this` and a value sets the value of the property,
* by calling appropriate method on `this`. If null, then the value will be set directly and
* may not be retrieved by the getter.
* @param attributes the attributes to set on the property
*/
public void defineProperty(
Context cx,
String name,
java.util.function.Function<Scriptable, Object> getter,
BiConsumer<Scriptable, Object> setter,
int attributes) {
if (getter == null && setter == null)
throw ScriptRuntime.typeError("at least one of {getter, setter} is required");

slotMap.compute(
name,
0,
(id, index, existing) ->
ensureLambdaAccessorSlot(
cx, id, index, existing, getter, setter, attributes));
}

private LambdaAccessorSlot createLambdaAccessorSlot(
Object name,
int index,
Slot existing,
java.util.function.Function<Scriptable, Object> getter,
BiConsumer<Scriptable, Object> setter,
int attributes) {
LambdaAccessorSlot slot;
if (existing == null) {
slot = new LambdaAccessorSlot(name, index);
} else if (existing instanceof LambdaAccessorSlot) {
slot = (LambdaAccessorSlot) existing;
} else {
slot = new LambdaAccessorSlot(existing);
}

slot.setGetter(this, getter);
slot.setSetter(this, setter);
slot.setAttributes(attributes);
return slot;
}

private LambdaAccessorSlot ensureLambdaAccessorSlot(
Context cx,
Object name,
int index,
Slot existing,
java.util.function.Function<Scriptable, Object> getter,
BiConsumer<Scriptable, Object> setter,
int attributes) {
var newSlot = createLambdaAccessorSlot(name, index, existing, getter, setter, attributes);
var newDesc = newSlot.getPropertyDescriptor(cx, this);
checkPropertyDefinition(newDesc);

if (existing == null) {
checkPropertyChange(name, null, newDesc);
return newSlot;
} else if (existing instanceof LambdaAccessorSlot) {
var slot = (LambdaAccessorSlot) existing;
var existingDesc = slot.getPropertyDescriptor(cx, this);
checkPropertyChange(name, existingDesc, newDesc);
return newSlot;
} else {
var existingDesc = existing.getPropertyDescriptor(cx, this);
checkPropertyChange(name, existingDesc, newDesc);
return newSlot;
}
}

protected void checkPropertyDefinition(ScriptableObject desc) {
Object getter = getProperty(desc, "get");
if (getter != NOT_FOUND && getter != Undefined.instance && !(getter instanceof Callable)) {
Expand Down Expand Up @@ -2695,6 +2781,17 @@ private static LambdaSlot ensureLambdaSlot(Object name, int index, Slot existing
}
}

private static LambdaAccessorSlot ensureLambdaAccessorSlot(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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? :)

Object name, int index, Slot existing) {
if (existing == null) {
return new LambdaAccessorSlot(name, index);
} else if (existing instanceof LambdaAccessorSlot) {
return (LambdaAccessorSlot) existing;
} else {
return new LambdaAccessorSlot(existing);
}
}

private void writeObject(ObjectOutputStream out) throws IOException {
out.defaultWriteObject();
final long stamp = slotMap.readLock();
Expand Down
Loading
Loading