Update static getProperty(Scriptable, Symbol) to no longer throw when not SymbolScriptable

[tonygermano]

Opening this as a draft PR as a result of comment #1611 (comment) and the resulting discussion.

I made SymbolScriptable now extend Scriptable to enforce the assumption stated in the interface description that classes which implement SymbolScriptable should also implement Scriptable. A SymbolScriptable which isn't also Scriptable would be pretty useless. I have not run the full test suite yet, but this did not break anything during compilation.

I moved the static getProperty methods which operate on Scriptables and utilize only interface methods to the Scriptable interface itself from ScriptableObject and deprecated them in ScriptableObject. It made more sense to me to locate them there, understanding that when these were originally written, interfaces did not support static methods.

If everyone seems ok with this approach, I'll do the same to move additional static methods and update the calling references elsewhere in the code. I'll make similar adjustments to the symbol versions of the hasProperty and setProperty methods according to the previously mentioned discussion.

[p-bakker]

Think this needs some unit test

[rbri]

If everyone seems ok with this approach

Yes please...do it

[gbrail]

If this simplifies a bunch of other code, and it should, then this makes sense to me too. It's especially nice if we can avoid having JavaScript code break because some internal thing does not implement SymbolScriptable.

[gbrail]

Yep, in an interface the "public" is irrelevant and lots of times we leave it out.

…

On Wed, Sep 11, 2024 at 6:09 AM Tony Germano ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In rhino/src/main/java/org/mozilla/javascript/Scriptable.java <#1615 (comment)>: > @@ -33,10 +33,10 @@ public interface Scriptable { * * <p>See ECMA 8.6.2 and 15.2.4.2. */ - public String getClassName(); + String getClassName(); From the Java Spec: https://docs.oracle.com/javase/specs/jls/se11/html/jls-9.html#jls-9.4 A method in the body of an interface may be declared public or private ( §6.6 <https://docs.oracle.com/javase/specs/jls/se11/html/jls-6.html#jls-6.6>). If no access modifier is given, the method is implicitly public. It is permitted, but discouraged as a matter of style, to redundantly specify the public modifier for a method declaration in an interface. SymbolScriptable (a newer interface) already omitted the public modifiers, but Scriptable did not. I was adding additional methods to Scriptable and wanted to keep them consistent. — Reply to this email directly, view it on GitHub <#1615 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I22XPEP2WAZYHF3LRVDZWA6KFAVCNFSM6AAAAABN2XE6VCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOJWHE2TONBSGU> . You are receiving this because you commented.Message ID: ***@***.***>

[tonygermano]

After looking at this for a while, I determined the getProperty methods I moved actually implement the abstract operation Get(O,P), so I think I'm going to move them to AbstractEcmaObjectOperations instead of Scriptable. Considering the size of the PR already. I think I'm just going to leave it at that for now, but we probably do want to move additional methods out of ScriptableObject that don't really belong there.

[tonygermano]

I'm getting a little discouraged here discovering what most of you probably already knew about how much of the behavior for JS Objects is actually defined in the ScriptableObject class and the rest of the code's dependency on ScriptableObject.

Our Scriptable#get(String|int|Symbol, Scriptable) doesn't actually match up with O.[[Get]]( P, Receiver ) in the spec, which is supposed to call AO OrdinaryGet ( O, P, Receiver ). OrdinaryGet then implements the recursive lookup up the prototype chain using O.[[GetOwnProperty]], which our Scriptable interface does not even have.

This is made apparent by the fact that Object.getOwnPropertyNames requires a ScriptableObject. For example, NativeJavaObject is a Scriptable which does not extend ScriptableObject.

js> Object.getOwnPropertyNames(new java.util.HashMap())
js: uncaught JavaScript runtime exception: TypeError: Expected argument of type object, but instead had type object
        at (unknown):11

Our Scriptable#get family of operations behaves similarly to O.[[GetOwnProperty]] in that it does not look up the prototype chain, however, it only returns the value rather than the full property descriptor. From what I can tell, the Slot class and its subclasses are meant to represent property descriptors internally. But not all Scriptables use Slots, and the conversion of a Slot to a Scriptable property descriptor again depends on the ScriptableObject class.

The ScriptableObject#getProperty methods, which I am attempting to move in this PR, are similar to the GetOrdinary AO in that they do the recursive prototype lookup, except they use the non-standard Scriptable#get to do so.

So, I'm kind of stuck on how to move forward without breaking a bunch of stuff.

1. How can we ensure that all Scriptables implement all of the internal methods that all JS Objects should have without making changes to the Scriptable interface?
2. How can we accurately implement the JS Abstract Operations on Objects when they do not all have the required methods?
3. If we do add the standard internal methods and behaviors to only our own objects, do we need two different implementations of every operation depending on whether we have a "new" Scriptable vs an "old" Scriptable?

[tonygermano]

#1629 is another example I came across while looking into this where a method that should work on all Scriptables only works for ScriptableObjects.

[p-bakker]

This is one of the challenges to tackle I guess, moving Rhino guard. I knew some of the issues, but you uncovered some more details 😛

I think we need some comprehensive plan to get to a s state we're happy with and the first step is determining what that state would look like

The Scriptable interface was defined a long, long, long time ago, when JavaScript didn't have all the bells and whistles it has nowadays, but since it's a public API, we're stuck with it, unless we either able to polyfill in missing behaviour, so Scriptable implementations out there in the wild with automatically become compatible, or we introduce a (big) breaking change'

Having said that, EcmaScript allows custom host objects that don't follow the 'rules' of EcmaScript

On the flipside: what is a Scriptable that doesn't extend ScriptableObject or what is it not? It's a host object, exposing method and properties, but without PropertyDescriptors, so no way to freeze or seal, introspect the setup of the properties etc.

So, on plain Scriptable implementations, one should be able to call Object.keys and Object.getOwnPropertyNames, but not the get/define propertyDescriptor(s) methods and the Object.freeze/seal method. Whether you can add me properties to a plain Scriptable implementation really depends on the implementation and thus not something Rhino can know.

But I think that Rhino itself shouldn't create Scriptable that don't extend ScriptableObject: the NativeJavaXxxxc family could extend ScriptableObject I think and would be always sealed (cannot add/remove properties), support propertyDescriptors and thus could possibly be frozen

We should make all relevant methods support non-ScriptableObject Scriptables and throw TypeErrors in others that cannot support plain Scriptables, when called on/with a plain Scriptable

All this probably means moving some code around, branching in some method to properly deal with non-ScriptableObject Scriptables, but looking at it from s high level it seems doable to me

[gbrail]

I feel the pain about Scriptable and ScriptableObject! It's a big change for someone but if anyone is brave enough to write up a proposal I think that we should keep talking about it.

[gbrail]

While I'm on my soapbox about this, trying to use the same class (ScriptableObject) to represent both an object and also to represent the current scope has also seemed crazy to me and also inefficient. I've often thought about trying to break that apart but I'm not sure it's possible...

[gbrail]

Any updates on this? It seems like a nice way to clean up, if we can reconcile it with other things that people are doing.