Implements compilation of new parameter types #449
Conversation
This initial commit enables compilation of shallow objectPattern and arrayPattern parameters. Shallow means that all elements of the objectPattern and arrayPattern are identifierParameters and particularly not nested object- or arrayPatterns
|
Can you also support such code:
|
- Improved clarity in CompilerError message - Improved brevity of tryStatement code because IdentifierParameter is the only allowed parameter - None case is in parameter type switch statement
| @@ -1047,21 +1047,37 @@ final class TestIn: JsOperation { | |||
|
|
|||
| } | |||
|
|
|||
| enum ParameterPattern { | |||
| case identifier | |||
| case object(properties: [ObjectPatternProperty]) | |||
There was a problem hiding this comment.
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.
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.
Ah hm I see. But does the order matter? Shouldn't these behave in the same way:
function foo({a, b}) {}
function foo({b, a}) {}
Or am I missing something?
There was a problem hiding this comment.
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.
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 👍
| var params = Parameters( | ||
| count: totalParameterCount | ||
| ) | ||
| params.patterns = patterns |
There was a problem hiding this comment.
Nit: can the constructor take patterns and then keep them immutable (i.e. as a let) instead?
There was a problem hiding this comment.
Will work on this now.
There was a problem hiding this comment.
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.
+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 the randomParameters() 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 "outer" view: the argument types that a caller need to use, and
- The "inner" view: the types of the parameters that the callee can use
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 like function 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?
The problem that is addressed with this commit can be seen when using testFunctionLifting. The test fails because the expected f0(a1) is instead f0() - i.e. somehow the parameter a1 wasn't lifted. This happens because the patterns array in the new Parameter array was initialized empty and only populated afterwards. In a later commit we will make patterns a constructor parameter and immutable afterwards.
| @@ -842,6 +842,36 @@ class LifterTests: XCTestCase { | |||
| } | |||
| b.reassign(f, to: f2) | |||
| b.callFunction(f) | |||
| var arrayParameter = Parameters(count: 2) | |||
There was a problem hiding this comment.
Maybe it would be nicer to move this into a new testcase, just to keep the size of the individual tests smaller so their are more readable. Maybe just a testParameterLifting() below this test?
|
Ah, another place that needs updating now: and and (i.e. the logic for converting between the prototuf-based serialized FuzzIL format to the in-memory one). I think this currently doesn't fail any tests because there is no CodeGenerator for the new parameters yet (and so they are not used in the respective tests), but you can probably observe the issue if you import a .js file with the new parameter types and then look at the .fuzzil file on disk (e.g. viaswift run FuzzILTool --liftToJS path/to/file.fuzzil).
|
This initial commit enables compilation of objectPattern and arrayPattern parameters