fix #1300 Support ES2019 Function.toString()

[tuchida]

Closes #1300
I have changed the result of calling toString for the following functions as specified.

/* built-in function */
Math.pow.toString()
// function pow() {
// 	[native code]
// }
//

/* bound function */
(function(){}).bind().toString()
// function bound () {
// 	[native code]
// }
//

/* Java Method */
java.math.BigInteger.valueOf.toString()
// function valueOf() {
// 	[native code]
// }
//

/* getter/setter */
Object.getOwnPropertyDescriptor({ get p() {} }, 'p').get.toString()
// get p() {}

Previously, the results were as follows.

/* built-in function */
Math.pow.toString()
// function pow() {
// 	[native code, arity=2]
// }
//

/* bound function */
(function(){}).bind().toString()
// function bound () {
// 	[native code, arity=0]
// }
//

/* Java Method */
java.math.BigInteger.valueOf.toString()
// function valueOf() {/*
// java.math.BigInteger valueOf(long)
// */}
// 

/* getter/setter */
Object.getOwnPropertyDescriptor({ get p() {} }, 'p').get.toString()
// p() {}

[tonygermano]

I'd like to request not implementing this change for Java methods. I don't believe they need to conform to the spec since they come from additional libraries which are not part of javascript and are allowed to override the .toString() method. The current behavior is very useful for enumerating all of the method signatures of overloaded java methods, which is sometimes required to access the correct method when the call is ambiguous.

For example,

js> java.util.Arrays.copyOf.toString()
function copyOf() {/*
float[] copyOf(float[],int)
int[] copyOf(int[],int)
byte[] copyOf(byte[],int)
long[] copyOf(long[],int)
char[] copyOf(char[],int)
boolean[] copyOf(boolean[],int)
double[] copyOf(double[],int)
java.lang.Object[] copyOf(java.lang.Object[],int)
short[] copyOf(short[],int)
java.lang.Object[] copyOf(java.lang.Object[],int,java.lang.Class)
*/}
js> i = java.util.Arrays['copyOf(int[],int)']([1.3, 2.7], 2)
[I@47eaca72
js> i[0]
1
js> i[1]
2
js> typeof i[0]
number
js> d = java.util.Arrays['copyOf(java.lang.Object[],int)']([1.3,2.7], 2)
[Ljava.lang.Object;@48f2bd5b
js> d[0]
1.3
js> d[1]
2.7
js> typeof d[0]
object
js> d[0].class
class java.lang.Double

[tuchida]

I'd like to request not implementing this change for Java methods.

I too think this is a controversial change. I made the change for two reasons. First, reading the spec, it is probably correct to return [native code] if it is neither user-defined nor built-in.
https://262.ecma-international.org/15.0/index.html#sec-function.prototype.tostring

2. If func is an Object, func has a [[SourceText]] internal slot

This is a user-defined case.

3. If func is a built-in function object,

This is a built-in case.

4. If func is an Object and IsCallable(func) is true, return an implementation-defined String source code representation of func. The representation must have the syntax of a NativeFunction.

And Java methods is supposed to be a case of 4.

The second reason was that GraalJS returned [native code].

> Java.type('java.math.BigInteger').valueOf.toString()
function valueOf() { [native code] }

[tonygermano]

Is my understanding correct that it isn't necessarily java.math.BigInteger.valueOf.toString() which is bound by the spec, but Function.prototype.toString.call(java.math.BigInteger.valueOf) instead? They can return different values, right?

I'd be fine with changing it for Function.prototype.toString.call(java.math.BigInteger.valueOf) only.

For example, in Chrome, I can do:

>  Math.random.toString()
<- 'function random() { [native code] }'
>  Math.random.toString = () => "override"
<- () => "override"
>  Math.random.toString()
<- 'override'
>  Function.prototype.toString.call(Math.random)
<- 'function random() { [native code] }'

I realize that is a user override, but I am just using it as an example that the toString method on the object isn't bound by the spec as long as Function.prototype.toString returns the expected value.

What does GraalJS return for an overloaded method like java.util.Arrays.copyOf as I showed in my example above, and how does it handle calling specific implementations of an overloaded Java method?

Also, I'm not sure if it makes a difference, but it looks like both Chrome and GraalJS return single-line strings for [native code] functions.

[tuchida]

What does GraalJS return for an overloaded method like java.util.Arrays.copyOf as I showed in my example above, and how does it handle calling specific implementations of an overloaded Java method?

I am not familiar with GraalJS either. By default, there seems to be a limit to the classes that can be used, and java.util.Arrays could not be used.

graaljs-24.0.2-linux-amd64$ ./bin/js 
> Java.type("java.util.Arrays")
TypeError: Access to host class java.util.Arrays is not allowed or does not exist.
	at <js> :program(<shell>:1:1:0-28)

[tonygermano]

I was trying to see if there was another way to easily enumerate the signatures of an overloaded method using javascript.

I think I uncovered a bug, too. It is correct that NativeJavaClass presents itself as a function, because it is a constructor. However, Object.prototype.toString knows that it is not a normal Function, and I can call Object.getOwnPropertyNames on constructors that I define in javascript without throwing any errors.

js> Object.keys(java.math.BigInteger)
ZERO,valueOf,ONE,TEN,probablePrime,TWO
js> 'valueOf' in java.math.BigInteger
true
js> 'valueOf(long)' in java.math.BigInteger // not enumerable
true
js> Object.getOwnPropertyNames(java.math.BigInteger) // oops
js: uncaught JavaScript runtime exception: TypeError: Expected argument of type object, but instead had type function
        at (unknown):57

js> typeof java.math.BigInteger
function
js> Function.prototype.toString(java.math.BigInteger)
function () {
        [native code, arity=0]
}

js> Object.prototype.toString.call(java.math.BigInteger) // not [object Function], can a `JavaMethod` also be a special type?
[object JavaClass]

I think the only other way to do this is getting messy with java reflection, something like this.

function getSignatures(nativeJavaClass, methodName) {
    return nativeJavaClass.__javaObject__.getMethods()
        .filter(function(method) {
            return (method.getName().equals(methodName)) && 
                (method.getModifiers() & java.lang.reflect.Modifier.STATIC)
        })
        .map(String)
        .join('\n')
}

Of course, that would need to be slightly modified if you wanted to list instance methods on a NativeJavaObject instead of static methods on a NativeJavaClass, both of which are nicely handled currently by the javascript toString method on a NativeJavaMethod.

[tuchida]

js> Function.prototype.toString(java.math.BigInteger)
function () {
        [native code, arity=0]
}

Is this what you wanted to do?

js> Function.prototype.toString.call(java.math.BigInteger)       
js: uncaught JavaScript runtime exception: TypeError: Method "toString" called on incompatible object (org.mozilla.javascript.NativeJavaClass is not an instance of org.mozilla.javascript.BaseFunction).
	at (unknown):2

js> Object.prototype.toString.call(java.math.BigInteger) // not [object Function], can a `JavaMethod` also be a special type?
[object JavaClass]

This would be consistent with the spec if it were modified to set Symbol.toStringTag.

Although compatibility breakdown is a problem, I think more interoperability with GraalJS and Nashorn would increase the number of users.

[tonygermano]

Is this what you wanted to do?

Oops, I thought I had caught and fixed that. I did have it correct in this comment #1537 (comment).

Like I said, I think the Function.prototype.toString output is what matters in relation to the spec, and that appears to be failing outright.

I think probably the best solution would be to introduce a new object in the prototype chain that sits between the object and Function.prototype, and that object can implement the current toString method for NativeJavaMethod.

Here is an example of how this could work. I know my Function.prototype.toString implementation here is too rudimentary, but it serves its purpose for this example:

js> function test() {
  >     const result = []
  >     var v = java.lang.Integer.valueOf
  >     result.push(v.toString()) // 1 - current behavior
  >     Object.getPrototypeOf(v) === Function.prototype
  >     var p = new Function()
  >     var oldToString = v.toString
  >     Object.setPrototypeOf(v, p)
  >     Function.prototype.toString = function() { return 'function ' + this.name + '() { [native code] }'}
  >     result.push(v.toString()) // 2 - Full prototype chain in place calling replaced Function.prototype.toString
  >     p.toString = oldToString
  >     result.push(v.toString()) // 3 - Calling original toString behavior as a property of the new prototype object
  >     result.push(Function.prototype.toString.call(v)) // 4 - Returns "specification" output 
  >     return result.join('\n--------------\n')
  > }
js> test()
function valueOf() {/*
java.lang.Integer valueOf(java.lang.String,int)
java.lang.Integer valueOf(java.lang.String)
java.lang.Integer valueOf(int)
*/}

--------------
function valueOf() { [native code] }
--------------
function valueOf() {/*
java.lang.Integer valueOf(java.lang.String,int)
java.lang.Integer valueOf(java.lang.String)
java.lang.Integer valueOf(int)
*/}

--------------
function valueOf() { [native code] }

This would not be unprecedented as GeneratorFunctions also have an intermediate prototype object. Although, in our implementation the GeneratorFunction prototype looks to have Object.prototype as its direct prototype rather than Function.prototype.

This leads to the odd result of

js> (function*(){}) instanceof Function
false

Whereas in Chrome that returns true.

[tuchida]

Function.prototype.toString override cannot return information except Java methods.
This is not a compatible proposal, How about defining own Symbol in every Java Wrapper that returns Java information?

// The Symbol to get a string of information about the Java being wrapped, or some object like __javaObject__
Symbol.javaXXX;

// For example
java[Symbol.javaXXX]; // An Object or a String, like [JavaPackage java]
java.util.HashMap[Symbol.javaXXX]; // [JavaClass java.util.HashMap]
(new java.util.HashMap())[Symbol.javaXXX]; // [JavaObject java.util.HashMap]
(new java.util.HashMap()).get[Symbol.javaXXX]; // [JavaMethod java.lang.Object get(java.lang.Object)]

This would allow us to see information other than methods and would not interfere with the ECMA 262 specification.

[tonygermano]

I'm not opposed to creating a java Symbol, but the section of the specification you are citing is specifically for the Function.prototype.toString method. That's the only thing that needs to change.

I could easily write a function that violates the spec in any javascript engine if it the spec applied to f.toString().

function f(i) { return i + 1 }
f.toString = () => "hidden"
f.toString() // returns "hidden"
Function.prototype.toString.call(f) // this is what matters to the specification

The actual Function.prototype.toString method needs to be able to recognize a JavaMethod and return the expected value without throwing an exception complaining it is not an instance of BaseFunction. That's an implementation detail, and we are still calling a JavaMethod a Function because it is callable, currently has Function.prototype as its prototype, and has a typeof "function." Otherwise, the spec wouldn't apply to it at all.

I believe it could work by defining the JavaMethodPrototype object in a similar manner to the Date.prototype object. Again, in Chrome

function test() {
    var d = new Date()
    var p = Object.getPrototypeOf(d)
    console.log(d.hasOwnProperty('toString'))
    console.log(p.hasOwnProperty('toString'))
    console.log(p === Date.prototype)
    d.toString = () => 'unavailable'
    console.log(Object.prototype.toString.call(d))
    console.log(Date.prototype.toString.call(d))
    console.log(d.toString())
}
>  test()
false
true
true
[object Date]
Fri Jul 26 2024 11:07:11 GMT-0400 (Eastern Daylight Time)
unavailable

Anyone who is relying on the string value of an unknown function in their program should be using Function.prototype.toString specifically to avoid any user-defined values in the same way that Object.prototype.toString has been historically used to get the "class" of the object.

From what I gather from the original proposal, the whole point of this change is that Function.prototype.toString should return a standardized value for functions which do not have source available that will throw an exception when trying to eval the result.

Additionally, if we do use a Symbol.toStringTag property to label it as a JavaMethod, that should be set on the JavaMethodPrototype object as well.

To use generators as an example, in Chrome (but not in Rhino!)

>  g = (function*(){})
<- ƒ* (){}
>  g[Symbol.toStringTag]
<- 'GeneratorFunction'
>  g.hasOwnProperty(Symbol.toStringTag)
<- false
>  Object.getPrototypeOf(g).hasOwnProperty(Symbol.toStringTag)
<- true

[tuchida]

I am sorry that I do not understand your problem very well. But for now, the nature of the problem is completely different between a JavaScript engine overriding a build-in function and a JavaScript engine user overriding a build-in function. For example, if Function.prototype.call or Function.prototype.bind were overwritten, the user would be confused. It doesn't have to be a Java engine, so if you have an example of overriding the build-in function, please let me know.
As for Generator, there should be a test in test262, so let's modify each one individually. That will be separate from this Pull Request.

[tonygermano]

I'm not talking about overwriting Function.prototype.toString in any sense. I know I gave a lot of examples in order to support my conclusion, and in doing which I discovered some Rhino behaviors which should probably be fixed separately from this PR. I will try to summarize what I think is relevant to this PR below.

Currently, Object.getPrototypeOf(Packages.MyClass.myMethod) evaluates to the same object as Function.prototype.

I'm suggesting that,

• Object.getPrototypeOf(Packages.MyClass.myMethod) should return a built-in object not in our current implementation, which we will name JavaMethodPrototype.
• JavaMethodPrototype should have a function toString which will mimic the current output of Packages.MyClass.myMethod.toString()
• JavaMethodPrototype can have a Symbol.toStringTag property with a value of "JavaMethod"
• Object.getPrototypeOf(JavaMethodPrototype) should return Function.prototype
• Function.prototype.toString.call(Packages.MyClass.myMethod) should return the NativeFunction string output required by the specification you are trying to implement.

Additionally,

• Function.prototype.toString.call(Packages.MyClass) should return the NativeFunction string output required by the specification you are trying to implement. This is because an instance of NativeJavaClass is a callable object.
• Packages.MyClass.toString() throws an Error unless the class has a static toString method. That behavior does not need to change in order to implement this part of the specification, and my preference would be that it remains unchanged.

Hopefully we can get some others to chime in with their opinion on this @gbrail @p-bakker @rbri .

[p-bakker]

Just for my info: the main reason you want a particular .toString implementation is to be able to discover all overloaded variants of a java method by calling .toString and manually reading the output while developing, instead of for example checking the relevant JavaDoc or using reflection?

[tuchida]

If Java method should have other prototype from JS function, should Java string and JS string also have other prototype?

[tonygermano]

If Java method should have other prototype from JS function, should Java string and JS string also have other prototype?

NativeJavaMethod is a special class of functions that are specific to Rhino. They are not defined in the ecma-262 spec.

Javascript has String objects, which we implement as instances of NativeString. They have a prototype of String.prototype. Typical JS strings are a primitive type that behave as if String.prototype was their prototype.

Java Strings in javascript land are instances of NativeJavaObject, which normally have a null prototype, however Strings get special treatment and also have String.prototype as their prototype.

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

Lines 158 to 161 in cc302b4

	if (prototype == null && javaObject instanceof String) {
	return TopLevel.getBuiltinPrototype(
	ScriptableObject.getTopLevelScope(parent), TopLevel.Builtins.String);
	}

Edit: If for some reason it was desirable for java Strings to have special behavior for certain functions defined in String.prototype that differ from how they are implemented in String.prototype, then inserting a new prototype object into the chain would be a good and normal way to accomplish that. Prototype objects are basically how javascript does inheritance. To use the metaphor, the NativeJavaObject$String "class" would then "extend" JS String, and users have the ability to travel up the chain to call "super" methods of overridden functions if they choose.

Nothing about that would conflict with the specification because the NativeJavaObject instance wrapping a java.lang.String instance is not defined in the spec. It is not a javascript string or String, but its own unique object type that happens to borrow their prototype and have similar properties.

[tonygermano]

Just for my info: the main reason you want a particular .toString implementation is to be able to discover all overloaded variants of a java method by calling .toString and manually reading the output while developing, instead of for example checking the relevant JavaDoc or using reflection?

More or less, yes.

My main points with regard to NativeJavaMethod are:

1. The current implementation is useful.
2. The spec does not require it to change.
3. We typically avoid changing things unnecessarily.
4. As it stands, this PR does not actually fix the part that is required by the spec. I.e., the spec speaks specifically to the behavior of Function.prototype.toString, and it currently throws an exception when called with a NativeJavaMethod instance bound to this.

[tuchida]

I too recognize that the current implementation is useful and that this PR change will break compatibility. I have written in the comment why I included the commit on top of that. If you really want to keep it, I delete the commit. However, I think that Rhino's challenge is that specification violations will remain for convenience and compatibility. It's like the old Internet Explorer.

And you say that NativeJavaMethod is not specified in ECMA 262, but that is not true. JavaScript is a language that has developed primarily with browsers. Useful JavaScript execution environments always have functions defined by the host environment in addition to those explicitly defined in ECMA 262. Recall Web API, Node.js, etc. This is probably why the specification mentions If func is an Object and IsCallable(func) is true, in addition to user-defined and built-ins.
In fact, the Web API follow this (Graal.js was noted earlier).

// Firefox
document.createElement.toString()
// "function createElement() {
//     [native code]
// }"

I do not agree with the proposal to create JavaMethodPrototype. It is too ad-hoc to provide own prototype only for JavaMethod. If you make it, you should have it in java.lang.String, JavaPackage, JavaClass, etc. And I think JavaMethodPrototype.toString() should also return [native code].

[tonygermano]

Again, the part you linked to in the specification is speaking to the behavior of Function.prototype.toString. It says nothing about platformBuiltin.toString, which can have any output we want it to have.

[tuchida]

I haven't seen anything from ECMA 262 that says something like “it is recommended to override the toString for debugging purposes.”. Also, I am unaware of any runtime environment that overrides the specification.

[tonygermano]

It would not be overriding the specification. A Date is an object, but (new Date()).toString() does not return [object Date]. https://262.ecma-international.org/15.0/index.html#sec-object.prototype.tostring specifies the behavior of Object.prototype.toString, not anyObject.toString().

Likewise, you are looking in (emphasis mine)

20.2 Function Objects
20.2.3 Properties of the Function.prototype Object
20.2.3.5 Function.prototype.toString( )

This method performs the following steps when called:
...
4. If func is an Object and IsCallable is true...

An instance of a JavaMethod would most closely fall under https://262.ecma-international.org/15.0/index.html#sec-built-in-function-objects. Here it says,

The initial value of a built-in function object's [[Prototype]] internal slot is %Function.prototype%, unless otherwise specified.

So, the specification explicitly allows us to specify our own prototype for this sub-class of functions, and nowhere does it say "properties of built-in function objects shall not deviate from the behavior of the properties of the Function.prototype object."

[tonygermano]

To reiterate, in order to be in full compliance with the spec,

• java.math.BigInteger.valueOf.toString() can return anything we want it to return.
• Function.prototype.toString.call(java.math.BigInteger.valueOf) must return "function valueOf() { [native code] }"

[tuchida]

So, the specification explicitly allows us to specify our own prototype for this sub-class of functions, and nowhere does it say "properties of built-in function objects shall not deviate from the behavior of the properties of the Function.prototype object."

It is too ad hoc to create a new JavaMethodPrototype just to override toString. If we have to make strange implementations to comply with the specification, it is better to remain in violation of the specification.

[tonygermano]

It is too ad hoc to create a new JavaMethodPrototype just to override toString.

Well, it doesn't have to only implement toString. It would have the Symbol.toStringTag property if we choose to implement that. It would also make it possible to define a (enumerable) getter property JavaMethodPrototype.javaMethods so that java.math.BigInteger.valueOf.javaMethods could return an array of NativeJavaObject wrapped java.lang.reflect.Method. Or a method JavaMethodPrototype.disambiguate() that could return an object where the property keys are the method signature and the values are JavaMethods.

it is better to remain in violation of the specification

We are not in violation of the spec because of what is returned by java.math.BigInteger.valueOf.toString(). We are in violation of the spec because Function.prototype.toString.call(java.math.BigInteger.valueOf) throws an error.

[tuchida]

We are not in violation of the spec because of what is returned by java.math.BigInteger.valueOf.toString()

Is this about the current Rhino implementation? Or are we talking about after the introduction of JavaMethodPrototype? The comment was made after JavaMethodPrototype was created, correct? In my opinion, the current implementation violates the specification.

It seems to me that your comment is a mix of before and after JavaMethodPrototype creation. Maybe my writing is not good either. sorry.

[tuchida]

To summarize my opinion,

1. Current Rhino javaMethod.toString violates ECMA 262 specification.
   • fix #1300 Support ES2019 Function.toString() #1537 (comment)
2. I would like to correct the violation. But if you must, we can leave it.
3. Overriding Function.prototype.toString is not preferred. JavaScript has no such idiom.
   • fix #1300 Support ES2019 Function.toString() #1537 (comment)
4. JavaMethodPrototype is too ad hoc.
   • fix #1300 Support ES2019 Function.toString() #1537 (comment)

[p-bakker]

I agree with @tuchida that the current impl. violated the spec:

• @tuchide imho correctly identified that the applicable section of the spec is https://262.ecma-international.org/15.0/#sec-function.prototype.tostring item 4, which reads:

4. 4. If func is an Object and IsCallable(func) is true,
return an implementation-defined String source code representation of func.
The representation must have the syntax of a NativeFunction.

• The seemingly contradictory return an implementation-defined String source code vs. The representation must have the syntax of a NativeFunction is explained by this discussion in the Function.prototype.toString revision proposal, which basically boils down to: the only thing flexible about the output is the whitespace

So, it we want EcmaScript compatibility, which I think is what we're after, the current .toString impl. on Java methods needs to go...

In my personal opinion the use-case for the old implementation (basically reflection to discover all overloaded variants of a method) is rather slim and have no problem losing it. Instead, I rather see these issues fixed:

=>Object.getOwnPropertyNames(java.math.BigInteger)
org.mozilla.javascript.EcmaError: TypeError: Expected argument of type object, but instead had type function (internal_anon#1)

=>Object.getOwnPropertyDescriptor(java.math.BigInteger, 'valueOf')
org.mozilla.javascript.EcmaError: TypeError: Expected argument of type object, but instead had type function (internal_anon#1)

=>for (var x of java.math.BigInteger) console.log(x)
org.mozilla.javascript.EcmaError: TypeError: [JavaClass java.math.BigInteger] is not iterable (internal_anon#1)

So you could do something like Object.getOwnPropertyDescriptors(java.math.BigInteger) and then find the info you're looking for

[tuchida]

=>Object.getOwnPropertyNames(java.math.BigInteger)
org.mozilla.javascript.EcmaError: TypeError: Expected argument of type object, but instead had type function (internal_anon#1)

=>Object.getOwnPropertyDescriptor(java.math.BigInteger, 'valueOf')
org.mozilla.javascript.EcmaError: TypeError: Expected argument of type object, but instead had type function (internal_anon#1)

=>for (var x of java.math.BigInteger) console.log(x)
org.mozilla.javascript.EcmaError: TypeError: [JavaClass java.math.BigInteger] is not iterable (internal_anon#1)

Let's fix this in another issue.

So you could do something like Object.getOwnPropertyDescriptors(java.math.BigInteger) and then find the info you're looking for

TypeError should be fixed, of course. In my opinion, when getting Java reflect information, it is better to add a special Symbol than to use JavaScript build-in functions. JavaScript property descriptors cannot express method overloads or access modifiers.

[p-bakker]

Let's fix this in another issue.

Definitely, wasn't suggesting to do it as part of this case

JavaScript property descriptors cannot express method overloads or access modifiers

Well, in Chrome Object.getOwnPropertyDescriptors(window) yields results as expected. If in Rhino such a call would yield propertyDescriptors for each individual overloaded method, I think it solves (at least a large chunk of) the issue @tonygermano has about discoverability of overloads, no?

jsSo for Object.getOwnPropertyDescriptors(java.util.Arrays) would yield, amongst others, propertyDescriptors for:

• copyOf(float[],int)
• copyOf(int[],int)
• copyOf(byte[],int)
• copyOf(long[],int)
• copyOf(char[],int)
• copyOf(boolean[],int)
• copyOf(double[],int)
• copyOf(java.lang.Object[],int)
• copyOf(short[],int)
• copyOf(java.lang.Object[],int,java.lang.Class)

The returnType would be missing though...

If going the 'Symbol' route, I think that ought to be an opt-in feature, not something enabled by default

[tuchida]

Can you display overloaded constructors? Well, there seems to be a lot to think about.

---

By the way, this is what the current implementation would look like.

js> java.math.BigInteger.toString()
js: Java class "java.math.BigInteger" has no public instance field or method named "toString".
	at (unknown):2

js> java.math.BigInteger
[JavaClass java.math.BigInteger]

The signatures of the eight constructors are not shown.

[p-bakker]

Added #1549 as a spin-off from this discussion

[tonygermano]

Sorry, I got really busy with other things and haven't been able to come back to this.

I can live with this change for now, but I really don't like not having an easy alternate way to get the same information ready to replace it, and any replacement will be less intuitive.

I apologize, I did find out that I was mistaken in thinking that Function.prototype.toString.call(nativeJavaMethod) was throwing an Error for much of this discussion. A string is returned, as expected.

However, an error is thrown for Function.prototype.toString.call(nativeJavaClass), which should be addressed as part of this PR.

I have some additional thoughts, which are obviously still up for discussion.

The majority of the logic to implement the spec should be here, which I'm pretty sure is where Function.prototype.toString actually lives.

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

Lines 320 to 325 in cc302b4

	case Id_toString:
	{
	BaseFunction realf = realFunction(thisObj, f);
	int indent = ScriptRuntime.toInt32(args, 0);
	return realf.decompile(indent, EnumSet.noneOf(DecompilerFlag.class));
	}

2. If func is an Object, func has a [[SourceText]] internal slot, func.[[SourceText]] is a sequence of Unicode code points, and HostHasSourceTextAvailable(func) is true, then
   a. Return CodePointsToString(func.[[SourceText]]).

We should implement some of these abstract methods, and I don't think we need to call decompile on function types that we know won't decompile.

NativeJavaClass is not an instance of BaseFunction, but it does implement Function, which extends the Callable interface, which I think is what determines whether a Scriptable IsCallable or not. So it won't have a decompile method, anyway.

I have not searched the code base to see if there are any other classes which implement Function or Callable, but are not sub-classes of BaseFunction.

A Callable should be able to answer all of the questions from step 2 of the spec above. Namely, we should be able to call the abstract HostHasSourceTextAvailable on any Callable, and be able to get the [[SourceText]] from any Callable. It can (probably?) be assumed that all Callables have a [[SourceText]] slot.

A Callable should be able to supply the information needed to construct a NativeFunction string, but not actually be the thing that returns the "[native code]" string. Basically the PropertyName via the [[InitialName]] internal slot, because it looks like neither Chrome nor Firefox display the FormalParameters. I.e.,

>  Math.abs.toString()
<- 'function abs() { [native code] }'
>  Math.abs.length
<- 1

Realizing that most of this was written well before Java 8, I wonder if some refactoring could be done to move some of the "Base" class functionality to interface default methods so that we can actually use the interfaces to tell what things are or what they support without losing all of the benefits of extending a base class.

[tuchida]

I can live with this change for now,

Thank you.

However, an error is thrown for Function.prototype.toString.call(nativeJavaClass), which should be addressed as part of this PR.

By the way, this is what happened in the JavaScript class.

// Firefox
class C { constructor(a) {} }

C.toString()
// "class C { constructor(a) {} }"

Function.prototype.toString(C)
// "function () {
//     [native code]
// }"

It might be a good idea to consider the relationship between JavaScript classes and Java classes to determine the specifications.

[tonygermano]

Firefox looks ok on my computer. To me this indicates that a user-defined class has a [[SourceText]] value.

This is what I get in Firefox 128.0.2 Snap for Ubuntu

class C { constructor(a) {} }

C.toString()
// "class C { constructor(a) {} }"

Function.prototype.toString.call(C)
// "class C { constructor(a) {} }"

Edit: I had a typo. Chrome 126.0.6478.132 on ChromeOS looks the same.

In both cases

Object.getPrototypeOf(C) === Function.prototype
// true

In the case of a NativeJavaClass it would not have a [[SourceText]] value, because it is a "built-in" and not javascript code, so it would not be the same as a user-defined javascript class.

[tonygermano]

I think it would be more like one of the Built-in objects that are also constructors.

// Chrome
typeof String
// 'function'
Object.getPrototypeOf(String) === Function.prototype
// true
Function.prototype.toString.call(String)
// 'function String() { [native code] }'

The main difference being that constructors which are instances of NativeJavaClass don't have a prototype.

// Rhino
js> typeof java.lang.String // "function" means it's callable
function
js> Object.getPrototypeOf(java.lang.String) // probably ok
null
js> Function.prototype.toString.call(java.lang.String) // this should work because it's an object and callable
js: uncaught JavaScript runtime exception: TypeError: Method "toString" called on incompatible object (org.mozilla.javascript.NativeJavaClass is not an instance of org.mozilla.javascript.BaseFunction).
        at (unknown):4

[tonygermano]

The reason I think it's ok that it doesn't have a prototype is because these are kind of special objects (Edit: I think the correct term may be "exotic" objects). For example, you can't call a .toString property on them unless the java class has a static toString method. They can only be coerced to strings.

js> java.lang.String.toString()
// js: Java class "java.lang.String" has no public instance field or method named "toString".
//        at (unknown):2

js> java.lang.Integer.toString() // toString property exists, but not with that method signature
// js: Can't find method java.lang.Integer.toString().
//        at (unknown):3

js> java.lang.Integer.toString // so helpful 😢
// function toString() {/*
// java.lang.String toString(int,int)
// java.lang.String toString(int)
// */}

js> String(java.lang.String)
// [JavaClass java.lang.String]
js> java.lang.String + 5
// [JavaClass java.lang.String]5
js> Object.setPrototypeOf(java.lang.String, Function.prototype)
// [JavaClass java.lang.String]
js> java.lang.String.toString
// js: Java class "java.lang.String" has no public instance field or method named "toString".
//         at (unknown):9

js> Object.getPrototypeOf(java.lang.String) // didn't actually set
// null

[tuchida]

@tonygermano @p-bakker Do you like the idea of using console.log (or cosole.info or console.debug) for debugging? There is no rule in the console.log specification for calling toString.

// Firefox
console.log(document.body);
// <body> <-- expandable

var obj = { a: 1 };
console.log(obj);
// Object { a: 1 }

// These are not the same as the result of toString
document.body.toString(); // "[object HTMLBodyElement]"
obj.toString(); // "[object Object]"

When NativeJavaClass/NativeJavaObject/NativeJavaMethod case, how about displaying useful information for debugging?

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

Lines 344 to 368 in cc302b4

	public Object call(
	Context callCx,
	Scriptable callScope,
	Scriptable callThisObj,
	Object[] callArgs) {
	Object value = callArgs[1];
	while (value instanceof Delegator) {
	value = ((Delegator) value).getDelegee();
	}
	if (value instanceof BaseFunction) {
	StringBuilder sb = new StringBuilder();
	sb.append("function ")
	.append(((BaseFunction) value).getFunctionName())
	.append("() {...}");
	return sb.toString();
	}
	if (value instanceof Callable) {
	return ScriptRuntime.toString(value);
	}
	if (arg instanceof NativeError) {
	return ((NativeError) arg).toString();
	}
	return value;
	}
	};

[p-bakker]

Rhino doesn't expose console by default (except in the shell) and outside of the shell one needs to provide an implementation.

Don't remember the details of providing an implements and whether the details of stringification of for example a function is within the realm of the part the the console implementation that Rhino provides, but if so, I guess that could be a good option

[tuchida]

Do you like the idea of using console.log (or cosole.info or console.debug) for debugging?

ref. #1554

[gbrail]

That was a very detailed discussion! Would one of you be willing to summarize this thread into a proposed action to take in either a modified version of this PR, or a new PR?

[tuchida]

I can live with this change for now, but I really don't like not having an easy alternate way to get the same information ready to replace it, and any replacement will be less intuitive.

From this comment, it seems that @tonygermano has tolerated this PR. I then proposed the idea of using console.log as an alternative. I would like input from @tonygermano and others on this idea.

[tuchida]

ref. https://github.com/mozilla/rhino/actions/runs/10294765267/job/28493340102?pr=1537
@p-bakker Probably due to #1540, but the test does not pass in Java 17. It seems to be caused by the different Unicode versions in Java 11 and Java 17. ref. #1383. Do you know of any good solutions?

[tonygermano]

Focusing on this PR, I think some of the points I made in #1537 (comment) still need to be addressed. Namely, Function.prototype.toString should work on all Callables per the specification, not only on instances of BaseFunction. I would like if additional functionality was created in the Callable interface to address this, and the implementation of Function.prototype.toString changed to follow the spec more closely and not assume that it only operates on BaseFunctions.

For example, we could add this method to Callable

default boolean hostHasSourceTextAvailable() { return false; }

Only the classes that do have the source available would need to override that method, and those are the only ones that we would need to decompile (which is a BaseFunction method that not all Callables have.) Otherwise, it would know to generate the NativeFunction output.

[tuchida]

@tonygermano I would like to see that addressed in another Pull Request.

[p-bakker]

Isn't the EcmaScript-defined internal hostHasSourceTextAvailable operation a thing in host level, instead of in individual 'functions'?

And I think we already have such a global flag somewhere, to exclude all source.

[p-bakker]

Probably due to #1540, but the test does not pass in Java 17. It seems to be caused by the different Unicode versions in Java 11 and Java 17.

Isn't it the other way around, it passes on Java 17, but not on Java 11? Our unicode depends on the Java implementation/version and both EcmaScript and Java include the latest unicode standard upon request. As such, unicode behavior in Rhino on an older Java version won't be 100% standard compliant

[tuchida]

@gbrail Thanks! I misunderstood the reason for the failure.

[tonygermano]

Isn't the EcmaScript-defined internal hostHasSourceTextAvailable operation a thing in host level, instead of in individual 'functions'?

It looks function level to me...

This is step 2 from the Function.prototype.toString section of the spec we are looking at for this PR. which is the only step that actually returns the source.

2. If func is an Object, func has a [[SourceText]] internal slot, func.[[SourceText]] is a sequence of Unicode code points, and HostHasSourceTextAvailable(func) is true, then
   a. Return CodePointsToString(func.[[SourceText]]).

And the definition from https://262.ecma-international.org/15.0/index.html#sec-hosthassourcetextavailable

The host-defined abstract operation HostHasSourceTextAvailable takes argument func (a function object) and returns a Boolean. It allows host environments to prevent the source text from being provided for func.

An implementation of HostHasSourceTextAvailable must conform to the following requirements:

It must be deterministic with respect to its parameters. Each time it is called with a specific func as its argument, it must return the same result.

The default implementation of HostHasSourceTextAvailable is to return true.

I was assuming that all Callables have a [[SourceText]] internal slot, which is empty for "native code". My thought was that HostHasSourceTextAvailable would return false for anything which would display as [native code], so it would be an easy boolean check before trying to actually get the SourceText (I didn't know how expensive the process was to generate the text.) Perhaps these would also be useful to add to Callable in order to implement Function.prototype.toString.

// maybe a different type? - "sequence of Unicode code points"
default Optional<String> getInternalSourceText() { return Optional.empty() }
// this one it actually refers to as a string
default Optional<String> getInternalInitialName() { return Optional.empty() }

The bottom line is that Function.prototype.toString needs to check if it is operating on a Callable which isn't a BaseFunction before trying to convert it to a BaseFunction. And in that case, it still needs to produce the NativeFunction output on something which will not have a BaseFunction::decompile method. So, that Callable will at least need to be able to provide the value of it's [[InitialName]] internal slot so that something else can generate the correct output.

[tuchida]

I will leave the rest to @tonygermano.

[tonygermano]

I will leave the rest to @tonygermano.

@tuchida I'm sorry, I did not mean to discourage you from working on this. If the goal of this PR is to make the function spec compliant, in reviewing I was finding places where it still was not spec compliant. I'd be happy to assist if you'd like to keep working on this.

If anyone else decides to pick it up, I found another case that still isn't working.

js> (function() {
    "use strict";
    let desc = Object.getOwnPropertyDescriptor(arguments, 'callee')
    return Function.prototype.toString.call(desc.get)
})()  >   >   >   > 
js: uncaught JavaScript runtime exception: TypeError: Cannot find default value for object.
        at (unknown):9 (anonymous)
        at (unknown):6

In chrome the output is 'function () { [native code] }'

[p-bakker]

Same here @tuchida, all feedback is just to get the correct thing implemented and I agree with you too not try to much in a single PR (and instead create follow-up cases where possible.

I really appreciate all the fixes you contributed lately and hope you'll be willing to reopen this PR to get it over the finish!

As for HostHasSourceTextAvailable: while the abstract operation in the spec takes a function as parameter, the idea behind the inclusion in the spec is just a global on/off switch for whether function.prototype.toString() returns the authored source code or just [native code]

As such, I think our current Context.setGeneratingSource(boolean is sufficient in this area, as the default impl. Of the abstract operation ought to always return true

[p-bakker]

@tonygermano the issue you described in #1537 (comment): is that new in this PR or an existing issue? Given the error you're getting, I wonder if this isn't a completely unrelated issue to the changes in this PR

[tonygermano]

@tonygermano the issue you described in #1537 (comment): is that new in this PR or an existing issue? Given the error you're getting, I wonder if this isn't a completely unrelated issue to the changes in this PR

The runtime exception is not caused by this PR, but this is an example of a different class implementing Callable which is not yet addressed by this PR. The spec says Function.prototype.toString should produce a string output (either the source text representation or the NativeFunction representation) for any Callable. It should throw a TypeError only when the object bound to this is not Callable.

There should be a test case for this class, even if the test is not currently passing.

[p-bakker]

Mmm, it seems I can't reopen this PR, not sure if that's a privilege issue or due to the fork from which this PR was made got deleted.

@gbrail or @rbri do either of you have the ability to reopen this PR?

[tonygermano]

I've got it in my repo at branch func.prototype.string if you want to clone it from there. https://github.com/tonygermano/rhino/tree/func.prototype.tostring

I did it a couple weeks ago and don't remember exactly how I did it, but I think there are similar instructions here https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally