Community
Participate
Working Groups
Build ID: I20080617-2000 Steps To Reproduce: 1. Pass null to new StructuredSelection(Object[]) 2. Watch NPE occur More information: The problem is there is no check on the the constructor for a null array. The javadoc doesn't specify that null is an illegal value, but looking at the class, it appears the other constructors do not allow null. Proposed fix: * Update the javadoc to say null is not allowed * Add an Assert.isNotNull to the constructor to match other constructors. Stacktrace: Thread [main] (Suspended (exception NullPointerException)) StructuredSelection.<init>(Object[]) line: 61 CategoriesView(SmalltalkBrowsingPart).selectAll(boolean) line: 864 CategoriesView.setSelection(IStructuredSelection, boolean) line: 274 CategoriesView(SmalltalkBrowsingPart).adjustInputAndSetSelection(Object) line: 797 CategoriesView.adjustInputAndSetSelection(Object) line: 253 CategoriesView(SmalltalkBrowsingPart).selectionChanged(IWorkbenchPart, ISelection) line: 731 CategoriesView.selectionChanged(IWorkbenchPart, ISelection) line: 224 PageSelectionService(AbstractSelectionService).firePostSelection(IWorkbenchPart, ISelection) line: 179 AbstractSelectionService$2.selectionChanged(SelectionChangedEvent) line: 71 StructuredViewer$3.run() line: 842 SafeRunner.run(ISafeRunnable) line: 37 Platform.run(ISafeRunnable) line: 880 JFaceUtil$1.run(ISafeRunnable) line: 48 SafeRunnable.run(ISafeRunnable) line: 175 TreeViewer(StructuredViewer).firePostSelectionChanged(SelectionChangedEvent) line: 840 TreeViewer(StructuredViewer).handlePostSelect(SelectionEvent) line: 1153 StructuredViewer$5.widgetSelected(SelectionEvent) line: 1178 OpenStrategy.firePostSelectionEvent(SelectionEvent) line: 251 OpenStrategy.access$5(OpenStrategy, SelectionEvent) line: 245 OpenStrategy$3.run() line: 419 RunnableLock.run() line: 35 UISynchronizer(Synchronizer).runAsyncMessages(boolean) line: 133 Display.runAsyncMessages(boolean) line: 3800 Display.readAndDispatch() line: 3425 Workbench.runEventLoop(Window$IExceptionHandler, Display) line: 2382 Workbench.runUI() line: 2346 Workbench.access$4(Workbench) line: 2198 Workbench$5.run() line: 493 Realm.runWithDefault(Realm, Runnable) line: 288 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 488 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149 IDEApplication.start(IApplicationContext) line: 113 EclipseAppHandle.run(Object) line: 193 EclipseAppLauncher.runApplication(Object) line: 110 EclipseAppLauncher.start(Object) line: 79 EclipseStarter.run(Object) line: 382 EclipseStarter.run(String[], Runnable) line: 179 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: 45 DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 25 Method.invoke(Object, Object...) line: 612 Main.invokeFramework(String[], URL[]) line: 549 Main.basicRun(String[]) line: 504 Main.run(String[]) line: 1236 Main.main(String[]) line: 1212
Created attachment 114113 [details] Assert added and javadoc updated Patch makes the StructuredSelection(Object[]) constructor consistent with the others.
(In reply to comment #0) > The > javadoc doesn't specify that null is an illegal value, but looking at the > class, it appears the other constructors do not allow null. See http://www.eclipse.org/articles/article.php?file=Article-API-Use/index.html "Do not treat null as an object. Null is more the lack of an object. Assume everything is non-null unless the API specification says otherwise." I don't think we need to do anything here. Yes, throwing a different exception would be clearer, but we fail fast, so it is not like this would lead to potentially hard to find bugs. (This is also why we have the asserts in the other constructors, where we would not fail fast otherwise.)
(In reply to comment #2) > (In reply to comment #0) > > The > > javadoc doesn't specify that null is an illegal value, but looking at the > > class, it appears the other constructors do not allow null. > > See http://www.eclipse.org/articles/article.php?file=Article-API-Use/index.html > "Do not treat null as an object. Null is more the lack of an object. Assume > everything is non-null unless the API specification says otherwise." Sure - I'm not disagreeing that null is invalid, I'm asking that the javadoc be updated to match the javadoc in some of other constructors. Using Eclipse, you can easily see the javadoc while developing, but you won't necessarily remember things from the article. > > I don't think we need to do anything here. Yes, throwing a different exception > would be clearer, but we fail fast, so it is not like this would lead to > potentially hard to find bugs. (This is also why we have the asserts in the > other constructors, where we would not fail fast otherwise.) > I agree with the Asserts - they say "I know this is an invalid input and I've marked it as such". An NPE, on the other hand, says something is wrong - it may be in my code or it may be in Eclipse. Making the change removes an ambiguity.
If you insist...
We would have to change a lot of JavaDoc if we wanted to fix this consistently. Not sure about the cost/benefit ratio though. Don't get me wrong, I do agree with your points and would recommend making constraints explicit in the Javadoc. Having to rely on an external "how to use the API" article does not work very well.
Verified by code inspection using I20081027-1300.
Comment on attachment 114113 [details] Assert added and javadoc updated added missing iplog+ flag.
