[rules-dev] BUG: [5.3.0.Final] CollectSetAccumulateFunction should probably use IdentityHashMap internally

[Wolfgang Laun]

    “When *I* use a word,” Humpty Dumpty said, in rather a scornful tone,
“it means just what I choose it to mean—neither more nor less.”
    “The question is,” said Alice, “whether you *can* make words mean so
many different things.”
    “The question is,” said Humpty Dumpty, “which is to be master      that’s
all.”

If a fact represents an entity with an immutable property set identifying
it and a mutable set of state properties I define equals() to mean "is the
same entity as".

If a fact represents a proposition I require equals() - taking into account
all properties - to mean "declares the same as", and uphold the notion that
such a fact shouldn't be modified at all, just retracted.

You can collect java.util.Sets of both, but don't violate the respective
semantics.


And here's a quote from java.util.Set's javadoc: Note: Great care must be
exercised if mutable objects are used as set elements. The behavior of a
set is not specified if the value of an object is changed in a manner that
affects equals comparisons while the object is an element in the set.

I'd say that this caveat extends to the usage of collectSet().

Of course, properly catching NPEs - even if they result from abuse - should
be done whenever appropriate.

-W



On 27 February 2012 17:53, SirMungus <Patrick_Rusk at ssga.com> wrote:

>
> Mark Proctor wrote
> >
> > accumulate supports custom functions, just add your own version of
> > collectSet that uses IdentityMap.
> >
> I could do that, but please let me take one more stab, just in case I
> haven't been clear enough.
>
> 1) I am not suggesting that collectSet() return an identity-based set. I am
> content to have it return a normal set, though I had early confusion on
> that
> point.
>
> 2) The discussion above about stated vs. justified facts turns out to be a
> distraction to the main point of the bug that I believe to be in
> CollectSetAccumulateFunction.  My failure to understand the distinction is
> not germane to the bug.
>
> 3) I am not disagreeing with the value behind implementing equals() to only
> take into account invariant fields, though I am stating that goes beyond
> the
> explicity documentation of equals()/hashCode() and is explicitly *not*
> followed by many classes in the JRE (most notably, collections).
>
> There is nothing about the purpose of collectSet() that logically suggests
> that it should only work with objects that have invariant
> equals()/hashCode() functions. If five objects match a rule, they should be
> able to be collected into a set (which may end up with fewer than five
> objects). If things change such that only three facts match the rule, they
> should still be able to be collected into a set.  There's no compelling
> reason why that should only apply to objects with invariant equals(). And
> it
> seems to me that you rarely want to leave an NPE in code, rather than
> catching it and throwing a more intelligible exception (e.g., "Object to be
> removed from collectSet() was not found").
>
> If the internal maintenance of state in CollectSetAccumulateFunction used
> identity semantics and applied the equality semantics only when you asked
> it
> for its results, it would maintain the same external behavior (returning a
> "normal" equality-based set) without failing to work for objects with
> variant equals.
>
> If you Google around for the proper implementation of equals(), you will
> find many serious sites that discuss it at length, yet provide examples
> like:
>
> public boolean equals(Object aThat){
>  if ( this == aThat ) return true;
>  if ( !(aThat instanceof Car) ) return false;
>  Car that = (Car)aThat;
>  return
>    EqualsUtil.areEqual(this.fName, that.fName) &&
>    EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors) &&
>    EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage) &&
>    EqualsUtil.areEqual(this.fColor, that.fColor) &&
>    Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks);
> //array!
> }
>
> Ironically, that is explicitly against Wolfgang's suggestion above!
>
>
> --
> View this message in context:
> http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFunction-should-probably-use-IdentityHashMap-internally-tp3774079p3781313.html
> Sent from the Drools: Developer (committer) mailing list mailing list
> archive at Nabble.com.
> _______________________________________________
> rules-dev mailing list
> rules-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/rules-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/rules-dev/attachments/20120227/f55ae1dd/attachment.html