Fix bug with arrays with widened numeric properties #1981
Conversation
src/values/AbstractObjectValue.js
Outdated
| @@ -561,6 +573,9 @@ export default class AbstractObjectValue extends AbstractValue { | |||
| } | |||
| invariant(d1val instanceof Value); | |||
| invariant(d2val instanceof Value); | |||
| if (d1val === d2val) { | |||
There was a problem hiding this comment.
This looked like an obvious optimization here, unless I'm mistaken?
There was a problem hiding this comment.
Yes, but joinValuesAsConditional should also do this optimization, so while this is faster, it is also a teeny bit of code bloat.
There was a problem hiding this comment.
I actually found a case just now where joinValuesAsConditional's simplify step hits the limit and returns the abstract condition, even though d1val === d2val.
There was a problem hiding this comment.
It seems like something we should try to fix in joinValuesAsConditional. Any chance you can come up with a repro?
src/methods/get.js
Outdated
| if (typeof P === "string") { | ||
| // these are safe methods to allow, as they return a new array | ||
| // so we use the ordinary get for these cases. Reduce can be | ||
| // unsafe, but we check for that in the prototype method |
There was a problem hiding this comment.
Reduce is not guaranteed to return an array, and we removed that logic. Let's fix up the comment too.
There was a problem hiding this comment.
@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
The depcheck test was passing in this PR and in recent commits is failing. I'm not sure why, it started failing after I edited the comment. |
|
The depcheck test is flaky on Circle. Nikolai reported it the Flow team, but so far no enthusiasm from their part. To be fair, this a hard thing to debug. |
There was a problem hiding this comment.
@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| if (typeof P === "string") { | ||
| // these are safe methods to allow, as they return a new array | ||
| // so we use the ordinary get for these cases. | ||
| if (P === "map" || P === "slice" || P === "filter" || P === "concat") { |
There was a problem hiding this comment.
I thought we got rid of this whitelist?
There was a problem hiding this comment.
No, we need them. We need to access the array prototype methods, but only the ones that create new arrays.
Release notes: none
This PR fixes a bug where prototype methods on unknown arrays with widened numeric properties were incorrectly being used instead of following the route to filter which methods were safe or not. The functionality and logic for this was pulled out, into its own function so it can be re-used in places and tidied up.