IrritantSet needs unsigned right shift for GROUP4 and higher

[kriegaex]

@stephan-herrmann: Lately, you activated IrritantSet.GROUP3 in commit 4989c7f:

eclipse.jdt.core/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java

Lines 39 to 48 in 8004cb6

	// Group prefix for irritants
	public final static int GROUP0 = 0 << GROUP_SHIFT;
	public final static int GROUP1 = 1 << GROUP_SHIFT;
	public final static int GROUP2 = 2 << GROUP_SHIFT;
	public final static int GROUP3 = 3 << GROUP_SHIFT;
	// reveal subsequent groups as needed
	// public final static int GROUP4 = 4 << GROUP_SHIFT;
	// public final static int GROUP5 = 5 << GROUP_SHIFT;
	// public final static int GROUP6 = 6 << GROUP_SHIFT;
	// public final static int GROUP7 = 7 << GROUP_SHIFT;

Because AspectJ fork already used GROUP3 before, I moved our group up to GROUP4. But now, methods like

eclipse.jdt.core/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java

Lines 289 to 293 in 8004cb6

	public IrritantSet set(int singleGroupIrritants) {
	int group = (singleGroupIrritants & GROUP_MASK) >> GROUP_SHIFT;
	this.bits[group] |= (singleGroupIrritants & ~GROUP_MASK); // erase the group bits
	return this;
	}

yield group indexes of -4 instead of the expected positive value, leading to index-out-of-bounds exceptions. I am not sure it is 100% correct, but changing all right shift operations to unsigned right shift seems to solve the problem.

diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java
--- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java	(revision Staged)
+++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java	(date 1707721190782)
@@ -226,7 +226,7 @@
 	}
 
 	public IrritantSet clear(int singleGroupIrritants) {
-		int group = (singleGroupIrritants & GROUP_MASK) >> GROUP_SHIFT;
+		int group = (singleGroupIrritants & GROUP_MASK) >>> GROUP_SHIFT;
 		this.bits[group] &= ~singleGroupIrritants;
 		return this;
 	}
@@ -244,7 +244,7 @@
 	public void initialize(int singleGroupIrritants) {
 		if (singleGroupIrritants == 0)
 			return;
-		int group = (singleGroupIrritants & GROUP_MASK) >> GROUP_SHIFT;
+		int group = (singleGroupIrritants & GROUP_MASK) >>> GROUP_SHIFT;
 		this.bits[group] = singleGroupIrritants & ~GROUP_MASK; // erase group information
 	}
 
@@ -282,14 +282,14 @@
 	}
 
 	public boolean isSet(int singleGroupIrritants) {
-		int group = (singleGroupIrritants & GROUP_MASK) >> GROUP_SHIFT;
+		int group = (singleGroupIrritants & GROUP_MASK) >>> GROUP_SHIFT;
 		return (this.bits[group] & singleGroupIrritants) != 0;
 	}
 	public int[] getBits() {
 		return this.bits;
 	}
 	public IrritantSet set(int singleGroupIrritants) {
-		int group = (singleGroupIrritants & GROUP_MASK) >> GROUP_SHIFT;
+		int group = (singleGroupIrritants & GROUP_MASK) >>> GROUP_SHIFT;
 		this.bits[group] |= (singleGroupIrritants & ~GROUP_MASK); // erase the group bits
 		return this;
 	}

Please, consider this patch and tell me if it is correct. I do not know the code at all, I am not sure. But if so, feel free to commit and just add Co-authored-by: Alexander Kriegisch <[email protected]> to the commit message. I am currently busy on another branch and cannot easily create a PR.

[srikanth-sankaran]

@stephan-herrmann , thanks for following up

[stephan-herrmann]

Because AspectJ fork already used GROUP3 before, I moved our group up to GROUP4. But now, ...

once more, AspectJ and Object Teams are in the same boat, see https://gitlab.eclipse.org/eclipse/objectteams/objectteams/-/commit/f7e999fd8596154ec814bbb706924571f355e178?page=4#a2886c9b208acdadf0499f59322c3382a041c832_302_304

The signed overflow only occurs once group 4 is used, because that's when the high-bit (4 << 29 = 2^32 = 0x80000000) is set.

JDT hasn't yet reached this situation.

So, feel free to apply that change in AspectJ with or without quoting my authorship :)

[kriegaex]

Hi @stephan-herrmann. Thanks for the qualified feedback with valuable information. Let me recap the situation from my personal perspective, simultaneously asking you to reconsider and fix the bug upstream:

• We both noticed a clear bug in JDT Core, seemingly almost at the same time.
• 3 days ago, I committed a simple workaround with a to-do comment to remove all changes once it is fixed upstream.
• Now you are telling me, you fixed it 6 days ago already in another fork of JDT Core, albeit in a bigger change, parts of which might or might not be relevant for upstream and for me to cherry-pick from your fork, I have absolutely no idea.
• You seem to be a JDT Core committer, which I am not.
• You are closing this issue as "not planned", even though it is a clear bug that has workarounds or solutions in at least 2 forks already. The reason you give is that the bug only occurs once group 4 is used, which we both know.

I.e., it is a (known!) bug in JDT Core. Why would you refuse to fix it there? Imagine, in 3 or 7 years JDT Core starts using group 4. Someone else than you spends debugging time for a third time and fixes the problem once more. Then, all forks sync the change and maybe have to refactor to adjust to a slightly different way this is done, e.g. the 4 right-shift operations being factored out into an extra method (just making up an example).

Does that make much sense? Is is economic in the efficiency and effectiveness sense?

The bug is there, and the class header explicitly states that the class is designed to use up to 8 groups.

eclipse.jdt.core/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java

Lines 29 to 30 in a9231ed

	* Represent a set of irritant flags. Irritants are organized in up to 8 group
	* of 29, allowing for a maximum of 232 distinct irritants.

eclipse.jdt.core/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java

Lines 39 to 48 in a9231ed

	// Group prefix for irritants
	public final static int GROUP0 = 0 << GROUP_SHIFT;
	public final static int GROUP1 = 1 << GROUP_SHIFT;
	public final static int GROUP2 = 2 << GROUP_SHIFT;
	public final static int GROUP3 = 3 << GROUP_SHIFT;
	// reveal subsequent groups as needed
	// public final static int GROUP4 = 4 << GROUP_SHIFT;
	// public final static int GROUP5 = 5 << GROUP_SHIFT;
	// public final static int GROUP6 = 6 << GROUP_SHIFT;
	// public final static int GROUP7 = 7 << GROUP_SHIFT;

Why not fix it now? Then the next fellow developer could just "reveal subsequent groups as needed" instead of fixing a bug unknown to him but known to multiple forks afterwards.

[stephan-herrmann]

@kriegaex you beat me to it: meanwhile I, too, had thoughts of reframing the discussion: on the one hand, there is no wrong behaviour in JDT at this point, so if we call it a bug, it's dormant to this point. OTOH, I would need to promise that I will take care the very moment JDT opens GROUP4, but at that distant point of future I may no longer be aware/around.

So, yes I will reopen in order to make this part of the implementation safe for the future.

Historical note: I vaguely remember a discussion with Philippe Mulet (!) back in 2008, when Object Teams was the first to break the previous barrier of 64 irritants :) So - yes - we language extenders are driving things now and then.