Re: RFR:8191301:JavaImporter fails to resolve imported elements within functions, that contain too many statements

[Priya Lakshmi Muthuswamy]

Hi Attila,

I have modified the patch with your suggestions.

webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.02/

Thanks,
Priya

On 12/12/2017 8:49 PM, Attila Szegedi wrote:

Hi Priya,

This indeed looks much better, although I do have some remarks with regard to 
the the style of the code. Specifically, repetitions of identical code, as well 
as assignments inside predicates.

There are several cases of code that is repeating:
First is:

((NativeJavaImporter)...).createProperty(JSType.toString(name))

Which occurs twice. You can avoid this by creating a “private static 
NativeJavaImporter getJavaImporter(Object self)” method that either returns 
self or does the lookup in scope, finally throws the type error if it found 
nothing. Then __noSuchProperty__ can be simply written as:

return getJavaImporter(self).createProperty(JSType.toString(name));

You have a similar duplication of ((WithObject)obj).getExpression() in 
getJavaImporterInScope that you should avoid with an assignment to a (final) 
local variable.

Also note that this method could return NativeJavaImporter as it already tests 
the expression for being an instanceof NativeJavaImporter. Basically, push the 
(NativeJavaImporter) cast down into getJavaImporterInScope; it’ll look nicer 
from the point of view of types than if you have to cast its return value in 
the caller.

Assignment in if: that’s discouraged because visually it’s easy to overlook or 
mistake for a comparison check at a glance. Instead of

ScriptObject expression;
if (self instanceof ScriptObject && (expression  = 
getJavaImporterInScope((ScriptObject)self))!=null) {
     return 
((NativeJavaImporter)expression).createProperty(JSType.toString(name));
}

use

if (self instanceof ScriptObject) {
     final NativeJavaExporter expression = 
getJavaImporterInScope((ScriptObject)self);
     If (expression != null) {
         return ...
     }
}

Attila.

On Dec 12, 2017, at 1:32 PM, Priya Lakshmi Muthuswamy 
<priya.lakshmi.muthusw...@oracle.com> wrote:

Hi,

Kindly review. I have modified the fix to work with multiple with scopes.

webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.01/

Thanks,
Priya
On 12/5/2017 12:54 PM, Priya Lakshmi Muthuswamy wrote:

Hi Attila,

Thanks for review.
Yes when I checked with two with scopes as suggested(JavaImporter as outer), 
current fix doesn't work. I will work on that.

Thanks,
Priya
On 12/5/2017 12:12 PM, Attila Szegedi wrote:

Hm… this seems to be an issue with shared scope calls; that’s why it’s 
sensitive to the number of similar statements.

That said, here’s some thoughts:

1. Instead of

     if (self instanceof ScriptObject && ((ScriptObject)self).hasWithScope()) {

you should be able to just use

     if (self instanceof ScriptObject) {

As then you’ll evaluate getWithScopeObject and test it for being null anyway. 
This way, you avoid walking the prototype chain twice.

2. That said, I guess hasWithScope could be reimplemented simply as

     public boolean hasWithScope() {
         return getWithScopeObject() != null;
     }

as both have very similar code, so it’d reduce to it nicely. (I understand that 
you haven’t changed that, but since you were in the vicinity of that code, you 
might as wel do it… It’s also fine if you leave it alone as it is.)

3. One of the statements in the test is indented differently than the others.

4. What happens if there’s _two_ with scopes, and the JavaImporter is in the 
outer one? Does this fix still work? E.g.:

var imports = new JavaImporter(java.lang);
var dummy = { x: 42, y: 13 }
with (imports) {
      with (dummy) {
          function func() {
              System.out.println('a');
             System.out.println('a');
         System.out.println('a');
              System.out.println('a');
             System.out.println('a');
         System.out.println('a');
             System.out.println('a');
          };
          func();
     }
}

Attila.

On Dec 5, 2017, at 7:13 AM, Priya Lakshmi Muthuswamy 
<priya.lakshmi.muthusw...@oracle.com> wrote:

Hi,

please review JDK-8191301 : JavaImporter fails to resolve imported elements 
within functions, that contain too many statements

JBS : https://bugs.openjdk.java.net/browse/JDK-8191301
webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.00/

Thanks,
Priya