groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ocs@ocs" <...@ocs.cz>
Subject Re: suggestion: ImplicitSafeNavigation annotation
Date Tue, 14 Aug 2018 17:10:28 GMT
Jochen,

> On 14 Aug 2018, at 6:34 PM, Jochen Theodorou <blackdrag@gmx.org> wrote:
> [...]
>> For the moment, the best solution — far as I have been able to ascertain — consists
of [*]
>> (a) at launch, setting own DelegatingMetaClass subclass for a Null.metaclass; it
essentially would return null from invokeMethod, but still needs to process special cases
(like e.g., null.is <http://null.is>(foo)) explicitly;
> 
> so for general Groovy... how are we supposed to recognized in a transform if "is" has
been replaced?

It is definitely highly arguable, but in the very specific case of “null?.is(null)“, which
currently returns an absurd (though technically understandable) false value, I would rather
suggest to break the backward compatibility.

I can't really see anyone whose code would actually use "?.is", and moreover, rely on that
“null?.is(null)“ is valid and returns null (instead of true). Is there such a person in
the sweet world?

Anyway, the point is, I'd say, rather at the unimportant side, for I believe is() is being
won't be widely used anyway, given we got === and !=== now.

Thus, even if we decide that breaking backward compatibility even in this slightly insane
case is a big no-no, we just can keep the current behaviour of “null?.is(null)==null“
unchanged. There's no real harm; new code would simply use === instead, and old one would
work as before.

>> (b) since the above for some godforsaken reason does not work for property access
at all, still implement an ASTT with a ClassCodeExpressionTransformer to set expression.safe=true
for get/setProperty, guarding explicitly against the known problematic cases (e.g., super.getProperty).
> 
> are you saying x?.foo will NPE if x is null? Or that x?.getFoo() will NPE in that case?
Not sure how to read your comment.

Provided only (a) “the Null.metaclass; returning null from invokeMethod” is used and no
ASTT, then yes, it would NPE. Easiest thing on earth to try:

===
262 /tmp> <q.groovy                            
class q {
    static main(args) {
        // ExpandoMetaClass.enableGlobally() // I thought this is needed; seems not (though
my application use it anyway)
        def mc=new OCSNMC(org.codehaus.groovy.runtime.NullObject)
        mc.initialize()
        org.codehaus.groovy.runtime.NullObject.metaClass=mc

        println "null.foo() is OK: ${null.foo()==null}"
        println "null.foo we won't see: ${null.foo==null}"
    }
}

class OCSNMC extends DelegatingMetaClass {
    OCSNMC(Class clazz){
        super(clazz)
    }
    Object invokeMethod(Object object, String methodName, Object[] arguments) {
        null
    }
}
263 /tmp> /usr/local/groovy-2.4.15/bin/groovy q
null.foo() is OK: true
Caught: java.lang.NullPointerException: Cannot get property 'foo' on null object
java.lang.NullPointerException: Cannot get property 'foo' on null object
	at q.main(q.groovy:9)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
264 /tmp> 
===

>> For all I know, this probably would not work properly with @CompileStatic (which
I do not use at all myself, but others do frequently).
> 
> the result type could be a problem... Worth to check.

Definitely :)

>> Trust me, been there, done that. I am pretty darn sure it would be /infinitely/ easier
and, what's important, more reliable in the core with an explicit compiler support.
> 
> How about making a small github project to dump the current state there?

Not that easy, for my code is mixed up with other ASTT and runtime stuff; but I'll try to
make some simple proof-of-concept ASAP and send here.

Thanks and all the best,
OC


Mime
View raw message