Remove usages of deprecated (in Java 9 and above) Class#newInstance
#5483
Conversation
There was a problem hiding this comment.
Fine if it avoids a handful of deprecation warnings whenever we do switch the codebase to -source 11. The loss of checked exceptions from newInstance() calls was not actually a problem at all—if you are using reflection obviously you do not know or care what throws clause the constructor used.
| } catch (InvocationTargetException e) { | ||
| Throwable t = e.getCause(); | ||
| if (t instanceof RuntimeException) { | ||
| throw (RuntimeException) t; | ||
| } else if (t instanceof IOException) { | ||
| throw new UncheckedIOException((IOException) t); | ||
| } else if (t instanceof Exception) { | ||
| throw new RuntimeException(t); | ||
| } else if (t instanceof Error) { | ||
| throw (Error) t; | ||
| } else { | ||
| throw new Error(e); | ||
| } |
There was a problem hiding this comment.
Why checked exceptions were a terrible idea: Exhibit A
There was a problem hiding this comment.
Indeed. I was tempted to use Cy Young. That's probably what Kohsuke would have done back in the day. 😊 But I decided to keep it simple.
There was a problem hiding this comment.
This is probably a case when introducing a new Checked Exception throwing method AND deprecating the old one is reasonable. I do not intend to start a holywar here though :)
There was a problem hiding this comment.
In this case the appropriate code would really be more like
try {
ClassLoader cl = Jenkins.get().getPluginManager().uberClassLoader;
instance = (Lifecycle)cl.loadClass(p).getDeclaredConstructor().newInstance();
} catch (Throwable t) {
LOGGER.log(Level.WARNING, "Could not load requested ${hudson.lifecycle}", t);
instance = new ExitLifecycle();
}
timja
added
the
ready-for-merge
|
This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
basil
added
the
squash-merge-me
| } catch (InvocationTargetException e) { | ||
| Throwable t = e.getCause(); | ||
| if (t instanceof RuntimeException) { | ||
| throw (RuntimeException) t; | ||
| } else if (t instanceof IOException) { | ||
| throw new UncheckedIOException((IOException) t); | ||
| } else if (t instanceof Exception) { | ||
| throw new RuntimeException(t); | ||
| } else if (t instanceof Error) { | ||
| throw (Error) t; | ||
| } else { | ||
| throw new Error(e); | ||
| } |
There was a problem hiding this comment.
This is probably a case when introducing a new Checked Exception throwing method AND deprecating the old one is reasonable. I do not intend to start a holywar here though :)
Problem
The documentation for
Class#newInstanceincludes the following warning:For this reason, the
Class#newInstancemethod has been deprecated in Java 9 and above.Solution
We should always prefer
myClass.getConstructor().newInstance()to callingmyClass.newInstance()directly. The latter sequence of calls is inferred to be able to throw the additional exception typesInvocationTargetExceptionandNoSuchMethodException. Both of these exception types are subclasses ofReflectiveOperationException. This PR updates our usages accordingly.Bonus
In addition, I noticed this codebase was using a mixture of
ErrorandAssertionErrorfor "impossible" reflection errors. Error Prone recommends against this in favor ofLinkageErrorfor rethrowingReflectiveOperationExceptionexceptions as unchecked. While the reasoning isn't explicitly stated, presumably it is becauseLinkageErroris more specific. I've updated our usage to match this recommendation.Proposed changelog entries
N/A
Proposed upgrade guidelines
N/A
Maintainer checklist
Before the changes are marked as
ready-for-merge:Proposed changelog entriesare correctupgrade-guide-neededlabel is set and there is aProposed upgrade guidelinessection in the PR title. (example)lts-candidateto be considered (see query).