280047 - Not implementing Scriptable in Undefined

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Igor Bukanov]

Currently in a few places Rhino code contains checks like:

if (x instanceof Scriptable && x != Undefined.instance) {
   do_something()
}

If Undefined would not implement Scriptable, that code would be reduced to simple:

if (x instanceof Scriptable) {
   do_something()
}

The additional benefit is code reduction in Undefined.java since the class would
only need to implement Serializable interface and declare Undefined.instance method.

Clearly it would be incompatible with code that directly uses Undefined, but
since the class is not a part of public API, I suggest to try  this.

Comment 1

[Igor Bukanov]

Attachment: Implementation

Comment 2

[Igor Bukanov]

I committed the patch after tagging the code in CVS as
before_simpler_undefined_280047 to be able to remove patch if too many
compatibility problems happens.

Comment 3

[Mario Cormier]

Igor,

I recently updated to the latest CVS Rhino code, and started seeing a lot of
warnings reported:

RHINO USAGE WARNING: Missed Context.javaToJS() conversion:
Rhino runtime detected object org.mozilla.javascript.Undefined@11adeb7 of class
org.mozilla.javascript.Undefined where it expected String, Number, Boolean or
Scriptable instance. Please check your code for missig Context.javaToJS() call.

The problem in this case is that the error doesn't even come from Java code, but
rather from a JavaScript invocation.  In one case, I invoke the contructor on
one of my ScriptableObject sub-classes, which takes three parameters:

   public void jsConstructor(String a, String b, int c) {
      // ...
      if (c == 0) {
         // Assume not specified in JavaScript
      }
   }

My understanding (correct me if I'm wrong) is that you are allowed to omit later
arguments, thereby implementing many constructors as one (you can't have two
different constructors).  In any case, it worked just fine prior to your fix --
omitting the 'c' parameter from JavaScript resulted in an undefined value, which
would result in a value of zero for 'c', which I would handle in the constructor.

Was my understanding wrong?  Is there anything I should do to make this work? 
Should Rhino be fixed to handle such cases?