Description Carsten Hammer 2021-01-03 11:44:51 EST

Bug: Comparison of different types

Using pointer equality to compare a AbstractSourceLookupDirector with a
ISourceLookupParticipant in
org.eclipse.debug.core.sourcelookup.AbstractSourceLookupDirector.dispose()

This method uses using pointer equality to compare two references that
seem to be of different types. The result of this comparison will always
be false at runtime.

Rank: Scariest (3), confidence: Normal
Pattern: EC_UNRELATED_TYPES_USING_POINTER_EQUALITY
Type: EC, Category: CORRECTNESS (Correctness)

The code currently looks like this:

	public synchronized void dispose() {
		ILaunchManager launchManager = DebugPlugin.getDefault().getLaunchManager();
		launchManager.removeLaunchConfigurationListener(this);
		launchManager.removeLaunchListener(this);
		for (ISourceLookupParticipant participant : fParticipants) {
			//director may also be a participant
			if(participant != this) {
				participant.dispose();
			}
		}
		fParticipants.clear();
		if (fSourceContainers != null) {
			for (ISourceContainer container : fSourceContainers) {
				container.dispose();
			}
		}
		fSourceContainers = null;
		fResolvedElements = null;
	}

Spotbugs complaints because it cannot know that there are (maybe) derived classes implementing ISourceLookupParticipant calling it. In general it is not such a good idea to have a dispose() call exception depend on the calling class implementing an interface used in a not-type-safe-way like here. It is difficult to read and may cause surprising side effects when something changes with the interfaces implemented. In this case it might be needed to prevent a recursion(?), not sure - hope it does not call a synchronized section with each call - did not dig that deep.

I'm not sure if anything is wrong here - spotbugs calls it scary - and I think at least that is justified. However if we are not able to change the code because lots of other more important issues we still might be able to add a comment so that spotbugs users might be notified in the moment they find this.

Here some information provided by Thomas:

Bug 29890 contains the attachment with the patch to introduce that change.
https://bugs.eclipse.org/bugs/attachment.cgi?id=7581

https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/173468 contains some more discussions.