Description Timo Kinnunen 2015-06-06 08:38:09 EDT

Using a debug aid function to check that Comparators implement their API contract correctly reveals that org.eclipse.mylyn.internal.tasks.ui.workingsets.TaskWorkingSetPage$CustomSorter has a bug and also uses a shortcut in its implementation, resulting in a violation.

The API contract states: "The implementor must ensure that sgn(compare(x, y)) == -sgn(compare(y, x)) for all x and y." 

Details from the debug aid method:
message: compare(x, y) is -1 but compare(y, x) is not 1, it is -1.

x was Tasks (org.eclipse.mylyn.internal.tasks.ui.workingsets.TaskWorkingSetPage$ElementCategory) at 0,

y was Resources (org.eclipse.mylyn.internal.tasks.ui.workingsets.TaskWorkingSetPage$ElementCategory) at 1
this: org.eclipse.mylyn.internal.tasks.ui.workingsets.TaskWorkingSetPage$CustomSorter
comparator: java.text.RuleBasedCollator
array:
	Tasks (org.eclipse.mylyn.internal.tasks.ui.workingsets.TaskWorkingSetPage$ElementCategory)
	Resources (org.eclipse.mylyn.internal.tasks.ui.workingsets.TaskWorkingSetPage$ElementCategory)

Examining the CustomSorter code reveals that when e1 is not an ElementCategory and e2 is, the result is a ClassCastException due to what appears to be a copy-paste error. When e1 and e2 both are an ElementCategory but e2 is the Tasks one, the bug causes execution to leave the short-circuiting if-statements and default to the super-class comparison method as if neither of compared objects was this special case object.

To fix the implementation the if-statements could be replaced with the following code:

			int result =
				Boolean.compare(
					e2 instanceof TaskRepository || e2 instanceof TaskRepositoryProjectMapping,
					e1 instanceof TaskRepository || e1 instanceof TaskRepositoryProjectMapping);
			if(result == 0) {
				result =
					Boolean.compare(
						e2 instanceof ElementCategory &&
						((ElementCategory) e2).getLabel(e2).equals(Messages.TaskWorkingSetPage_Tasks),
						e1 instanceof ElementCategory &&
						((ElementCategory) e1).getLabel(e1).equals(Messages.TaskWorkingSetPage_Tasks));
			}
			if(result == 0) {
				result = super.compare(viewer, e1, e2);
			}
			return result;