Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implements compilation of new parameter types #449
base: main
Are you sure you want to change the base?
Implements compilation of new parameter types #449
Changes from 5 commits
5fb8f98
2a0816b
6e8fde4
5905832
d4169b7
3466c11
97fa1fb
6d57a33
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit: can the constructor take
patterns
and then keep them immutable (i.e. as alet
) instead?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.
Will work on this now.
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.
The more I think about it, the more I'm starting to feel this might be a bigger problem.
Changing the constructor requires updating all places in the codebase where we instantiate Patterns. Even if we use a default value for backward compatibility, shouldn’t we still review every instance in the entire codebase where Parameters is referenced and ask: “What should be the patterns argument for the Parameters constructor?”
I've also noticed there are separate data structures, Parameter and ParameterList, which might also need revision—along with all their references in the codebase.
These extensive changes might be needed again once rest parameters and default values are supported.
The point I’m trying to make is that it feels like a lot of things might need to change for this, but the effort should also be justified and prioritized. I’m thinking it would make the most sense for me to focus on implementing the delete operator in Compilation and string object keys first, as they are likely easier to implement and will together enable hundreds of new seeds to become compilable.
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.
+1 to working on delete operator and string object keys, those should allow for some quick wins!
I think it would make sense to have a convenience constructor of Parameters that creates a simple list of parameters: no patterns, no rest parameter, etc. just
function foo(a1, a2, a3) {
. Then we can have a second constructor for all the fancy stuff (patterns, rest param, etc.). I would think that the majority of use cases in Fuzzilli want the first constructor. Most of the time that should be the right thing to do. Only if you actually want to stress the logic for handling the special parameter types in the target engine would you want the other constructor. And that should then mostly be limited to the Compiler and a few CodeGenerators (or I guess therandomParameters()
helper function).Regarding the ParameterList, I think this one doesn't need to change, but maybe its documentation should be updated. The way I see it, there are basically two use cases for the types of a function's parameters:
The ParameterList struct (which is part of a Signature) is used for both of these cases. In both cases, we still only need a "flat" list of types: one for each argument or parameter. However, up until now, the two "views" would be identical: if the 2nd parameter was of type .integer, then that's what the caller had to pass and what the callee could assume. Now, for the caller the 2nd parameter might be of type
.object(withProperties: ["a", "b"])
while for the callee, the 2nd parameter might be of type.integer
(because it's something likefunction foo(p, {a: v3, b: v4})
. So the same function can have different ParameterLists for the two "views". I think it's fine to keep using the same data structures for that, just the distinction should maybe be described in a comment? Alternatively, we could try to force the distinction by using two different data structures. I'm not sure that will work well though, but might be worth a try. WDYT?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.
Could this also be a
[String: ParameterPattern]
dictionary or would that make things more complicated? (asking because that "feels" a bit more natural without having thought about it deeply)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.
Unfortunately switching to a dictionary based approach causes problem because it does not contain information about the order of the object property keys. The dictionary tells us which value belongs to which key but not in which order the keys appear in the object pattern. This is why I opted for an array.
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.
Ah hm I see. But does the order matter? Shouldn't these behave in the same way:
Or am I missing something?
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.
Technically yes. The order of the parameters doesn't matter. However the mapping from parameter -> variable is lost this way (IIUC). I.e. the order of variables is fixed but the order of object properties is permutated.
We can do it anyway by using an array of tuples instead of a dictionary. This way we can retain the orderedness.
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.
Right yeah that's a good point. It also needs to be consistent across serialization and deserialization (see #449 (comment)) so I guess a list of tuples is the way to go here 👍