RFR:8191301:JavaImporter fails to resolve imported elements within functions, that contain too many statements
Attila Szegedi
szegedia at gmail.com
Wed Dec 13 08:35:34 UTC 2017
Excellent!
+1
Attila.
> On 2017. Dec 13., at 4:42, Priya Lakshmi Muthuswamy <priya.lakshmi.muthuswamy at oracle.com> wrote:
>
> Thanks Attila, I have modified the patch
>
> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.03/ <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 <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