ClassCastException when using Combinators during shrinking #475
Comments
|
@SimY4 Thanks for reporting. Can you add a short example reproducing the phenomenon? First thought: Changing type signatures without changing the implementation shouldn’t be able to get rid of a CCE if it already compiled before. But I may be wrong. |
|
@SimY4 I couldn't reproduce it on my own. Waiting for your input then. |
|
@jlink Sorry, took me a while to pin it down. I'm still trying to create a minimal reproducible example for this bug. But my current understanding of the problem is: I have an entities with identity equality defined like so: abstract class Common {
private final String value;
protected Common(String value) { this.value = value; }
public boolean equals(Object o) { return this == o || (o instanceof Common && value.equals(((Common) o).value)); }
public int hashCode() { return value.hashCode(); }
}
class This extends Common {
static final This INSTANCE = new This();
This() { super(""); }
}
class That extends Common {
static final That INSTANCE = new That();
That() { super(""); }
}This and That types are different but their identity is the same, therefore:
So, when it comes to memorization that's built-in inside Jqwik, this right here: jqwik/engine/src/main/java/net/jqwik/engine/facades/Memoize.java Lines 28 to 34 in 1e45490 won't tell them apart. So one can be substituted with another. And that would break the runtime type checker. |
|
Based on the sample above, I would say What is the use case for having different classes that are equal in that way? Have you considered adding PS. I wonder if it would make sense to avoid caching |
|
@vlsi you might be right. But discovering that my somewhat broken equality is being used as a cache key also came as a surprise to me since I wasn't expecting jqwik to do that. I'd probably consider But the problem when goes higher because: record Product(Common p1, Common p2) {}
Combinators.combine(arbCommon(), arbCommon()).as(Product::new)are also the same now |
|
Caching is done on a heuristical basis and requires reasonable equality implementation. In a way this is a hack, but I haven't found a better solution yet. If caching was done on an identity basis it wouldn't have the intended effect. |
|
@jlink is there a configuration option to disable it? Maybe I can add that in |
Nope. Convince me that it would be a good idea to make caching configurable. The problem is that without generator caching, many non-trivial value generators will become VERY slow. |
WDYT of making the cache configurable (e.g. to allow more or less memory for the caching)?
The key issue here is that users supply I doubt they will know when it is the right time to call Even though handling
Of course, the caching would be sub-optimal, however, it might be a reasonable compromise between reasonably hard to support code, reasonably easy to use (less caching surprises), and reasonably fast to execute. WDYT?
@SimY4 , would you clarify the issue? I don't get it :-/ |
The simplest solution for this special case is to use identity hashing for Frankly, I'm not convinced it's time well spent to handle anomalies like that. There are probably dozens or hundreds of similar cases all over jqwik. |
|
Some afterthoughts that I had lately: |
jqwik relies on the cache for cases like See The preparation step has non-trivial overhead, so creating generators always does not sound good. |
I love that term :-). jqwik is 50% reflection magic. Some of it is shady. So you're right. My counter-argument is: It (mostly) works for the use cases I've seen so far. |
|
@vlsi I'm sorry, I may be overstepping here, but I don't see the need for what you just described. Isn't frequency about weighing your arbitraries and randomly selecting one based on that? I can't think of a reason for you to explode and weigh individual items. The frequency method you've shown is about frequencies of values and not arbitraries, but I'm too new to this codebase to maybe I'm not seeing what you're showing me. I still can't see how caching of arbitraries is useful. Are you trying to save on heap space? |
Weighted generators require preprocessing which is CPU-intensive. Caching enables running the pre-processing once rather than "for every generated item". |
Generators are cached, not arbitraries - based on the arbitrary which creates them. It's not about saving heap space but creation time. In addition to the example from @vlsi, there are many more. One prominent example is recursive arbitraries. Creating a new generator on each recursion step can be very time-consuming. Same holds for nested combinators. Without this kind of caching (or another approach to speed this up) quite a few use cases get out of reasonable performance bounds. All that said, is there a reason that the supposedly broken implementation of |
|
Just wanted to include this for future references, just in case we'd land on some outcomes from these discussions. I've fixed my equalities and hashcodes and now they aren't universal like they used to. Most of the |
|
Sorry, it was my bad with the last one. |
You mean it's not a jqwik issue? |
|
@jlink I can twist it that it is. 😀 The last one came from class Configurator extends ArbitraryConfiguratorBase {
public Arbitrary<MyType> configure(Arbitrary<MyType> arb, MyAnnotation ann) { ... }
}This looks like it a correct configurator. Except it isn't because you also shouldn't forget to override MyType.class.isAssignableFrom(targetType.getRawType());without it it can mess up all annotated arbitraries. But it's not relevant to the original issue, so it's irrelevant to the initial issue with cache collisions. |
This is exactly what I discovered yesterday when looking into "your" other issue: #493 At the time of implementing |
Testing Problem
ClassCastException when using Combinators and arbitraries producing elements of the type hierarchy.
Suggested Solution
Very brief analysis points to the lack of variance in functional combinator methods. i.e.:
should probably be relaxed to:
The text was updated successfully, but these errors were encountered: