Copilot commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4167963447
##########
drools-base/src/main/java/org/drools/base/reteoo/sequencing/signalprocessors/LogicGate.java:
##########
@@ -97,19 +118,24 @@ public void resetPrior(SequenceMemory memory,
ValueResolver valueResolver) {
gate.reset(memory, valueResolver);
}
- memory.resetLogicGateMemory(gateIndex, valueResolver);
+ if (!statusCanRevert) {
+ memory.resetLogicGateMemory(gateIndex, valueResolver);
+ }
}
public void reset(SequenceMemory memory, ValueResolver valueResolver) {
resetPrior(memory, valueResolver);
output.reset(memory, valueResolver);
}
- public void activate(SequenceMemory memory) {
+ public void activate(SequenceMemory memory, ValueResolver valueResolver) {
for (int i = 0; i < filterIndexes.length; i++) {
memory.activateSignalAdapter(filterIndexes[i], this,
signalAdapterIndexes[i], i + 1); // bit indexes start at 1
}
-
+ if (predicate.test(0L, allMatched)) {
Review Comment:
Activation evaluates the gate as if no child had matched, but child gates
are activated before their parents and zero-state gates can already signal the
parent. For example, in `xnor(xnor(a, b), c)` the inner XNOR sets one outer
input bit; the outer activation still evaluates `0` and incorrectly propagates
a match. Evaluate the gate's actual memory so nested XNOR state is respected.
##########
drools-base/src/main/java/org/drools/base/reteoo/sequencing/signalprocessors/LogicGateOutputSignalProcessor.java:
##########
@@ -46,4 +46,10 @@ protected void reset(SequenceMemory memory, ValueResolver
valueResolver) {
// by each downstream LogicGate resetting itself via resetPrior().
There is no
// state held here that needs clearing.
}
+
+ public void clearParentBit(SequenceMemory memory) {
+ long[] gateMemory = memory.getLogicGateMemory();
+ gateMemory[gate.getGateIndex()] = gateMemory[gate.getGateIndex()] &
~(1L << (index - 1));
+ memory.setLogicGateMatched(gate.getGateIndex(), false);
Review Comment:
This clears only the immediate parent's state. If that parent already
propagated to a grandparent, the grandparent retains a stale match bit. For
example, with `and(and(xor(a, b), c), d)`, matching `a` and `c`, then matching
`b` reverts the XOR but leaves the outer AND's inner-AND bit set, so `d`
incorrectly completes the step. Reversion needs to re-evaluate the parent and
propagate the transition recursively through the circuit.
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/DSL.java:
##########
@@ -555,7 +555,7 @@ public static FixedValueItem expr( String exprId, boolean
value ) {
// -- Existential operator --
- public static ExprViewItem not( ViewItemBuilder<?> expression,
ViewItemBuilder<?>... expressions) {
+ public static ExistentialExprViewItem not( ViewItemBuilder<?> expression,
ViewItemBuilder<?>... expressions) {
Review Comment:
Changing this public static method's return type changes its JVM descriptor,
so clients compiled against the previous `ExprViewItem not(...)` signature will
fail with `NoSuchMethodError` even though source recompilation succeeds.
Preserve the existing descriptor and expose sequence-step typing through a
binary-compatible API design.
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/impl/ViewPatternBuilder.java:
##########
@@ -194,6 +221,50 @@ public static Condition ruleItem2Condition(RuleItem
ruleItem) {
throw new UnsupportedOperationException( "Unknown " + ruleItem );
}
+ /**
+ * Rewrites bare xor()/xnor() steps in a sequence by folding each such step
+ * with the immediately following positive step into an and(xor(...),
next) pair.
+ * <p>
+ * {@code sequence(xor(a, b), c)} is legal sugar for {@code
sequence(and(xor(a, b), c))}.
+ * The trailing-XOR check in the caller will catch {@code sequence(xor(a,
b))} (no following step).
+ */
+ private static org.drools.model.SequenceStep[]
rewriteXorSteps(org.drools.model.SequenceStep[] steps) {
+ boolean hasXor = false;
+ for (int i = 0; i < steps.length; i++) {
+ org.drools.model.SequenceStep step = steps[i];
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type t = ((CombinedExprViewItem) step).getType();
+ if ((t == Condition.Type.XOR || t == Condition.Type.XNOR) && i
+ 1 < steps.length) {
+ hasXor = true;
+ break;
+ }
+ }
+ }
+ if (!hasXor) {
+ return steps;
+ }
+ List<org.drools.model.SequenceStep> result = new ArrayList<>();
+ for (int i = 0; i < steps.length; i++) {
+ org.drools.model.SequenceStep step = steps[i];
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type t = ((CombinedExprViewItem) step).getType();
+ if ((t == Condition.Type.XOR || t == Condition.Type.XNOR) && i
+ 1 < steps.length) {
+ // Wrap: and(xor/xnor(...), nextStep)
+ CombinedExprViewItem wrapped = new CombinedExprViewItem(
+ Condition.Type.AND,
+ new ViewItem[]{(ViewItem) step, (ViewItem) steps[i
+ 1]});
+ result.add(wrapped);
+ i++; // consume the following step
Review Comment:
The rewrite consumes any following step, not necessarily the required
positive step. Thus `sequence(xor(a,b), xor(c,d), e)` becomes
`sequence(and(xor(a,b), xor(c,d)), e)`: the second XOR is no longer folded with
`e`, and the first step can advance before `e`. Validate the next step as
positive or normalize chained absence/reverting gates without consuming their
intended trigger.
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/impl/ViewPatternBuilder.java:
##########
@@ -185,7 +185,34 @@ public static Condition ruleItem2Condition(RuleItem
ruleItem) {
if (ruleItem instanceof SequenceViewItem) {
SequenceViewItem sv = (SequenceViewItem) ruleItem;
- List<Condition> steps = Arrays.stream(sv.getSteps())
+ org.drools.model.SequenceStep[] rawSteps =
rewriteXorSteps(sv.getSteps());
+ if (rawSteps.length > 0) {
+ org.drools.model.SequenceStep last = rawSteps[rawSteps.length
- 1];
+ if (last instanceof ExistentialExprViewItem) {
+ ExistentialExprViewItem ex = (ExistentialExprViewItem)
last;
+ if (ex.getType() == Condition.Type.NOT) {
+ throw new IllegalArgumentException(
+ "sequence(): trailing not() or nor() requires a
following positive step " +
+ "or a completeWithin(...) deadline. " +
+ "Trailing absence with completeWithin is planned
but not yet implemented. " +
+ "See ADR 0002.");
+ }
+ }
+ if (last instanceof CombinedExprViewItem) {
+ CombinedExprViewItem comb = (CombinedExprViewItem) last;
+ if (comb.getType() == Condition.Type.NOT
+ || comb.getType() == Condition.Type.NOR
+ || comb.getType() == Condition.Type.XOR
+ || comb.getType() == Condition.Type.XNOR) {
Review Comment:
The trailing-gate guard only checks the outer node, so wrapping a reverting
gate bypasses it. `sequence(and(xor(a, b)))` is accepted, then terminates as
soon as the first child matches; the second child can never revert it,
defeating the exactly-one-before-a-positive-trigger contract. Validate the
effective condition tree and require a genuine positive trigger rather than
relying on the top-level type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
