Catch IAE when Gradle error getLocation cannot be called.

[lkishalmi]

I've bumped into this one today:

java.lang.IllegalArgumentException: object is not an instance of declaring class
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.netbeans.modules.gradle.loaders.LegacyProjectLoader.getLocation(LegacyProjectLoader.java:380)

Couldn't really know how to reproduce. I was fiddling with old Gradle versions with the new tooling.

The fix is trivial though...

[mbien]

the problem might be deeper than this. The method accepts throwable but expects it to be LocationAwareException. If someone would call it with the wrong exception type

            if (locationAccessor == null) {
                locationAccessor = locationAwareEx.getClass().getMethod("getLocation"); // NOI18N
            }

would intitialize the field wrong if it happens to have that method, and all subsequent calls will fail or do really weird things.

I personally am not a fan of Throwable usage, since it is in 99% of all cases not something you should be catching.

[lkishalmi]

Well, I think it's a tradeoff we need to pay when we try to cover multiple versions at once. My mind could trick me, but some exceptions had getLocation methods even before LocationAwareException was introduced.

This is just some heuristic, trying to figure out what's behind a Gradle build failure. If we can get a location out of that we display it as an editor hint on the source code.

Well, if the heuristic does not work we just shall let it go...

[mbien]

but doesn't the Method field has to fit to the right class? If this method is supposed to be as generic as possible and work for all exceptions which have a getLocation method, I don't think it should be cached as field. It should be checked if that exception has getLocation on every invocation:

            Method locationAccessor = locationAwareEx.getClass().getMethod("getLocation"); // NOI18N
            return (String)locationAccessor.invoke(locationAwareEx);

[mbien]

this illustrates the issue:

        Integer five = 5;
        Double pi = Math.PI;
        
        Method stringAccessor =  five.getClass().getMethod("toString");
        System.out.println(stringAccessor.invoke(pi));

[lkishalmi]

Point taken. Haven't really checked what's in the try block. Thanks!

[mbien]

looks good to me

[sdedic]

Let me try a different fix ... I've cached the accessor so that the reflective access is not that slow; but it should be easy to check for instanceof LocationAwareException. It seems that I might do a completely wrong thing with this reflective stuff, as the LocationAwareEx type is in the same place since Gradle 3.1 or even more ... give me 24hrs to check, thanks.

[mbien]

added do not merge label so that nobody merges by accident

[lkishalmi]

@sdedic Take your time!

Just would like to add, that it's fine not to cache the Method accessor here. This is on a critical path.

[sdedic]

Ouch ! No caching is possible, in fact. During debugging, I've realized (only now) that the Tooling API seems to load classes for the deserialized data using a throwaway classloader , so Classes from 2 project loads are not assignable at all, even though they represent the same type.

[mbien]

@sdedic thanks a lot for checking! Reflection is super fast anyway these days - I don't think much is lost there.