-
Notifications
You must be signed in to change notification settings - Fork 163
feat: add @ElementOf annotation #1028
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
8058002 to
4aabeac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces the @ElementOf annotation to allow restricting generated values to a fixed set of constants during mutation testing. The annotation supports all primitive types (and their boxed equivalents) as well as String.
- Adds
@ElementOfannotation with type-specific array fields (bytes, shorts, integers, longs, chars, floats, doubles, strings) - Implements
ElementOfMutatorFactoryto create mutators that select from the provided value sets - Updates documentation with usage examples and annotation reference table
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/code_intelligence/jazzer/mutation/annotation/ElementOf.java | New annotation definition with array fields for each supported type |
| src/main/java/com/code_intelligence/jazzer/mutation/mutator/lang/ElementOfMutatorFactory.java | Factory implementation that creates mutators for selecting from fixed value sets |
| src/main/java/com/code_intelligence/jazzer/mutation/mutator/lang/LangMutators.java | Registers the new ElementOfMutatorFactory in the mutation chain |
| src/test/java/com/code_intelligence/jazzer/mutation/mutator/lang/ElementOfMutatorFactoryTest.java | Unit tests verifying integer and string selection behavior, plus validation |
| src/test/java/com/code_intelligence/jazzer/mutation/mutator/StressTest.java | Integration test case for string array selection |
| docs/mutation-framework.md | Documentation updates with annotation reference and usage example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/com/code_intelligence/jazzer/mutation/annotation/ElementOf.java
Show resolved
Hide resolved
b4ddf7a to
64ffa36
Compare
64ffa36 to
8b05445
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition, thanks!
It works great if the user never removes @ElementOf annotation from the type.
My main issue is that in the corpus we save indices and not the actual values.
This is especially bad for strings if the user's intention is to help the fuzz test boost coverage, and then delete the @ElementOf annotation.
Doing so instantly splits the coprus into two parts---one that gives you coverage with @ElementOf and one without it. You cannot join them.
It is ok-ish only for integers, that have the same byte size, because the bytes that come after are still correctly aligned.
For the other integers, the rest of the bytes is misaligned and probably worthless.
The order of the elements matters, because the corpus only contains indices.
Inserting an element in-between the element list will invalidate the corpus that contains indices for all elements after.
I think, by saving the actual values in the corpus, instead of just indices, we could make this annotation more efficient for users who tend to frequently tweak their fuzz test signature. The only tricky part is to de/serialize strings, but it will make this annotation a lot better. I have prototyped an implementation that does that, take a look at the branch element-of-copy if you like!
pre-approving this, but you know what to do!
src/main/java/com/code_intelligence/jazzer/mutation/mutator/lang/LangMutators.java
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/annotation/ElementOf.java
Show resolved
Hide resolved
| mutateIndices(values.size()), | ||
| values::get, | ||
| value -> { | ||
| int index = values.indexOf(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's useful to allow @ElementOf with single values.
Current restriction seems artificial.
This can be side-stepped by providing the same value >1 times):
@ElementOf(longs = {1L, 1L}) @NotNull Long i
Perhaps we could rename the old mutateIndices into mutateAtLeastTwoIndices or similar:
public static SerializingMutator<Integer> mutateAtLeastTwoIndices(int length) {
require(length > 1, "There should be at least two indices to choose from");
return mutateIndices(length);
}
public static SerializingMutator<Integer> mutateIndices(int length) {
require(length > 0, "There should be at least one index to choose from");
// old codeindexOf needs in worst case O(n) comparisons.
This is probably still faster than a hash map for small number of values, so it should be ok, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's useful to allow @ElementOf with single values.
Current restriction seems artificial.
This is mirroring the behavior of other mutators which fail if there is nothing to mutate. IMO it makes sense to ensure that every mutator can perform at least one useful operation to make sure that we don't waste performance on ineffectual mutations or allow fuzz tests that have effectively fixed inputs.
indexOf needs in worst case O(n) comparisons.
This is probably still faster than a hash map for small number of values, so it should be ok, right?
Hmm, I was thinking of smallish @ElementOf arrays but we can't be sure. So maybe a hash map that we construct once is the better option.
src/main/java/com/code_intelligence/jazzer/mutation/mutator/lang/ElementOfMutatorFactory.java
Outdated
Show resolved
Hide resolved
| format("@ElementOf(%s, size=%d) -> %s", fieldName, values.size(), targetTypeName)); | ||
| } | ||
|
|
||
| private static List<Byte> boxBytes(byte[] values) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to only save distinct values?
Currently @ElementOf(bytes={1,1,1,2}) has 4 values, but effectively only two.
And 1 is three times more likely to be picked than 2. Is this what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO tweaking the probabilities is the most intuitive option if a user wants to add duplicate values. Or do you see a problem with duplicate values?
8b05445 to
8e35e63
Compare
Great review, thanks! |
This allows picking from a fixed set of values during mutation.
8e35e63 to
c9c1053
Compare
Allows picking from a fixed set of values during mutation.