search

spotbugs / spotbugs-maven-plugin

Maven Mojo Plug-In to generate reports based on the SpotBugs Analyzer
https://spotbugs.github.io/spotbugs-maven-plugin/
Apache License 2.0
69 stars 51 forks source link

Add opens for Groovy ReflectionUtils #172

Closed wborn closed 4 years ago

wborn commented 4 years ago

Based on this StackOverflow post I've programmatically added some add-opens for the Groovy ReflectionUtils class. That resolves the warnings in #132 for me on Java 11. These are still not fixed in Groovy 3.0.0-rc-3 . :-/

wborn commented 4 years ago

If you think this is acceptable, I will probably still need to add some check/catch for Java <= 8 versions.

hazendaz commented 4 years ago

Its novel for sure but I dont think we should be doing this here. It would be better that groovy makes the appropeiate patches. Otherwise we wont know if they ever fix it and we could break on newer jdks. For now I'll leave this open so gage others interest in this. I do understand others not wanting to see the issue but honestly lots of tools still pump these errors and fixing too far from the real code as others on stack overflow stated is more risky.

Get Outlook for Androidhttps://aka.ms/ghei36

From: Wouter Born notifications@github.com Sent: Saturday, January 18, 2020 1:38:54 PM To: spotbugs/spotbugs-maven-plugin spotbugs-maven-plugin@noreply.github.com Cc: Jeremy Landis jeremylandis@hotmail.com; Review requested review_requested@noreply.github.com Subject: Re: [spotbugs/spotbugs-maven-plugin] Add opens for Groovy ReflectionUtils (#172)

If you think this is acceptable, I will probably still need to add some check/catch for Java <= 8 versions.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHubhttps://github.com/spotbugs/spotbugs-maven-plugin/pull/172?email_source=notifications&email_token=AAHODI3MPVG3PEZDVLJLC4TQ6NED5A5CNFSM4KIUNQ5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJJ7EIY#issuecomment-575926819, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAHODI55C6A6Q5W4HKOGKPTQ6NED5ANCNFSM4KIUNQ5A.

wborn commented 4 years ago

I also had a look at the debug stacktrace (3.1.12.2) and found that the warnings are caused by the debug logging:

WARNING: Illegal reflective access by org.codehaus.groovy.reflection.ReflectionUtils (file:/home/wouter/.m2/repository/org/codehaus/groovy/groovy/3.0.0-beta-2/groovy-3.0.0-beta-2-indy.jar) to method java.util.Collections$UnmodifiableCollection.toString()
    at org.codehaus.groovy.reflection.ReflectionUtils.makeAccessible(ReflectionUtils.java:204)
    at org.codehaus.groovy.reflection.ReflectionUtils.makeAccessible(ReflectionUtils.java:196)
    at org.codehaus.groovy.reflection.ReflectionUtils$1.run(ReflectionUtils.java:189)
    at org.codehaus.groovy.reflection.ReflectionUtils$1.run(ReflectionUtils.java:187)
    at java.base/java.security.AccessController.doPrivileged(Native Method)
    at org.codehaus.groovy.reflection.ReflectionUtils.makeAccessibleInPrivilegedAction(ReflectionUtils.java:187)
    at org.codehaus.groovy.reflection.CachedMethod.makeAccessibleIfNecessary(CachedMethod.java:380)
    at org.codehaus.groovy.reflection.CachedMethod.setAccessible(CachedMethod.java:155)
    at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoCachedMethodSite.<init>(PojoMetaMethodSite.java:182)
    at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoCachedMethodSiteNoUnwrapNoCoerce.<init>(PojoMetaMethodSite.java:207)
    at org.codehaus.groovy.reflection.CachedMethod.createPojoMetaMethodSite(CachedMethod.java:313)
    at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.createCachedMethodSite(PojoMetaMethodSite.java:159)
    at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.createPojoMetaMethodSite(PojoMetaMethodSite.java:148)
    at groovy.lang.MetaClassImpl.createPojoCallSite(MetaClassImpl.java:3525)
    at org.codehaus.groovy.runtime.callsite.CallSiteArray.createPojoSite(CallSiteArray.java:131)
    at org.codehaus.groovy.runtime.callsite.CallSiteArray.createCallSite(CallSiteArray.java:165)
    at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:47)
    at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:115)
    at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:119)
    at org.codehaus.mojo.spotbugs.SpotBugsMojo.executeSpotbugs(SpotBugsMojo.groovy:1029)
    at org.codehaus.groovy.runtime.callsite.PlainObjectMetaMethodSite.doInvoke(PlainObjectMetaMethodSite.java:43)
    at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite$PogoCachedMethodSiteNoUnwrapNoCoerce.invoke(PogoMetaMethodSite.java:190)
    at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite.callCurrent(PogoMetaMethodSite.java:58)
    at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:51)
    at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:156)
    at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:168)
    at org.codehaus.mojo.spotbugs.SpotBugsMojo.canGenerateReport(SpotBugsMojo.groovy:573)
    at org.codehaus.mojo.spotbugs.SpotBugsMojo.execute(SpotBugsMojo.groovy:708)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:137)
    at org.twdata.maven.mojoexecutor.MojoExecutor.executeMojo(MojoExecutor.java:119)
    at org.openhab.tools.analysis.tools.AbstractChecker.executeCheck(AbstractChecker.java:110)
    at org.openhab.tools.analysis.tools.SpotBugsChecker.execute(SpotBugsChecker.java:178)

So it's triggered by:

https://github.com/spotbugs/spotbugs-maven-plugin/blob/dd665f585b26dc79bd55504911b4ebbbe2a1dc8f/src/main/groovy/org/codehaus/mojo/spotbugs/SpotBugsMojo.groovy#L1029

When I make this debug logging conditional it won't get into that code block which will stop the warnings from showing (unless the log level is raised) and it will also prevent unnecessary concatenations.

See for example: https://github.com/wborn/spotbugs-maven-plugin/commit/a4231c04b6bb32858c16035e42bf9699ab031a7a

WDYT about such a workaround @hazendaz?

hazendaz commented 4 years ago

@wborn I like that solution better. It's clean and avoid the issue. I suspect if running debug one might want to know of the illegal reflection. Can you go ahead and send that over as a PR?

wborn commented 4 years ago

I've created https://github.com/spotbugs/spotbugs-maven-plugin/pull/173 for this.