RFR:8191301:JavaImporter fails to resolve imported elements within functions, that contain too many statements
Priya Lakshmi Muthuswamy
priya.lakshmi.muthuswamy at oracle.com
Wed Dec 13 03:42:08 UTC 2017
Thanks Attila, I have modified the patch
webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.03/
Thanks,
Priya
On 12/13/2017 12:57 AM, Attila Szegedi wrote:
> This is generally acceptable, and while I really don’t want to be a
> pain, I’ll point out few minor things still, because I really want
> this code to be the best it can be :-)
>
> - in Nashorn code (and OpenJDK in general), we format “else” and “else
> if” to be on the same line as the closing brace of the previous block,
> so :
>
> if (…) {
> …
> } else if (…) {
> …
> } else {
> …
> }
>
>
> - in Nashorn, we declare all method parameters to be final.
>
> - We strongly prefer final local variables with single-assignment
> whenever possible, so instead of early-initializing “expression" to
> null in getJavaImporter and then overwriting it, the preferred style
> would be:
>
> final NativeJavaImporter expression;
> if (self instanceof NativeJavaImporter) {
> expression = (NativeJavaImporter)self;
> } else if (self instanceof ScriptObject) {
> expression = getJavaImporterInScope((ScriptObject)self);
> } else {
> expression = null;
> }
> return expression;
>
>
> The compiler will prove that a final variable is assigned exactly once
> on each branch. This code is fairly simple so it might not matter
> much, but it’s good to adopt this style of coding as it lets the
> compiler help you ensure you didn’t forget to initialize the variable
> on any path. Sometimes the early-initialization value, in this case
> null, wouldn’t be valid in all cases and by providing it early, you
> paper over all the missing cases and the compiler can’t tell if you
> overlooked something. On the other hand, if you declare the variable
> as final, it’ll report a compilation error if the variable is not
> initialized on every possible path.
>
> Alternatively, you could even do away with expression completely, and
> just use early return statements like this:
>
> if (self instanceof NativeJavaImporter) {
> return (NativeJavaImporter)self;
> }
> if (self instanceof ScriptObject) {
> return getJavaImporterInScope((ScriptObject)self);
> }
> return null;
>
> but I don’t consider that to be more valid than the above version with
> "final NativeJavaImporter expression”; they’re pretty much equivalent,
> albeit this last one is slightly shorter.
>
> Attila.
>
>> On 2017. Dec 12., at 18:31, Priya Lakshmi Muthuswamy
>> <priya.lakshmi.muthuswamy at oracle.com
>> <mailto:priya.lakshmi.muthuswamy at oracle.com>> wrote:
>>
>> Hi Attila,
>>
>> I have modified the patch with your suggestions.
>>
>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.02/
>> <http://cr.openjdk.java.net/%7Epmuthuswamy/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.muthuswamy at oracle.com
>>>> <mailto:priya.lakshmi.muthuswamy at 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/
>>>> <http://cr.openjdk.java.net/%7Epmuthuswamy/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.muthuswamy at oracle.com
>>>>>>> <mailto:priya.lakshmi.muthuswamy at 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/
>>>>>>> <http://cr.openjdk.java.net/%7Epmuthuswamy/8191301/webrev.00/>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Priya
>>>>>>>
>>>>>>>
>>
>
More information about the nashorn-dev
mailing list