Make it easier to combine abstract values via operators. Part 1. #937
Conversation
src/domains/ValuesDomain.js
Outdated
| return new ValuesDomain(resultSet); | ||
| } | ||
|
|
||
| static computeBinary( |
There was a problem hiding this comment.
I tried to just keep this where it was in BinaryExpression, but that causes the Flow cycle length to increase. Arguably, this is the better place for it anyway.
src/domains/TypesDomain.js
Outdated
| @@ -36,6 +40,50 @@ export default class TypesDomain { | |||
| return this._type || Value; | |||
| } | |||
|
|
|||
| // return the type of the result in the case where there is no exception | |||
| static binaryOp(op: BabelBinaryOperator, left: TypesDomain, right: TypesDomain) { | |||
There was a problem hiding this comment.
is it possible to Flow-type the result?
src/domains/ValuesDomain.js
Outdated
| op: BabelBinaryOperator, | ||
| lval: ConcreteValue, | ||
| rval: ConcreteValue, | ||
| lloc: ?BabelNodeSourceLocation, |
There was a problem hiding this comment.
These parameters seem to be unused?
There was a problem hiding this comment.
Ah yes, copy pasta. Will remove.
src/domains/ValuesDomain.js
Outdated
| static binaryOp(realm: Realm, op: BabelBinaryOperator, left: ValuesDomain, right: ValuesDomain): ValuesDomain { | ||
| let leftElements = left._elements; | ||
| let rightElements = right._elements; | ||
| if (!leftElements || !rightElements || leftElements.size > 100 || rightElements.size > 100) |
There was a problem hiding this comment.
I thought you prefer === undefined over !?
There was a problem hiding this comment.
What's the reasoning behind the > 100? I have no clue, and don't see a comment.
There was a problem hiding this comment.
The ugliness of the pretty version of line 77 made me swallow ! for this instance.
I'll add a comment. The reason for the size restriction is that the larger a value set becomes, the less useful it becomes while at the same time it becomes computationally expensive.
src/domains/ValuesDomain.js
Outdated
|
|
||
| // 5. If Type(rval) is not Object, throw a TypeError exception. | ||
| if (!(rval instanceof ObjectValue)) { | ||
| throw realm.createErrorThrowCompletion(realm.intrinsics.TypeError); |
There was a problem hiding this comment.
Doesn't this object allocation and field initialization create some entries that are tracked by the realm? Just pointing it out, because the catch-logic in binaryOp seems to assume that computeBinary is pure. I'd prefer to make computeBinary truly pure.
There was a problem hiding this comment.
Good catch. This should throw a FatalError.
|
The Cartesian product computation code is pretty well covered by the existing tests, but I don't think any of them will fail if the value set is actually wrong. Coming up with such tests in the current state of the Abstract Interpreter is non trivial and I'd prefer to hold of a bit until it is easier to do so. |
|
@hermanventer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
In this part, the code for creating an abstract value that results from a binary operator is centralized into AbstractValue.createFromBinaryOp. As part of this, TypeDomain and ValuesDomain now have transfer functions for computing the domain values that result from binary operations.
If this pattern is acceptable, I'll extend it to the other operators.