Share types with deleted properties #5704
Conversation
| delete obj['a'] | ||
| TEST('{"c":3,"b":2,"d":4}', JSON.stringify(obj)); |
There was a problem hiding this comment.
This improves compat, right? Since it seems like we now more accurately have properties in insertion order?
In general, this should probably go through test262, since it seems like we have had a lot of corner cases with property deletion come up recently.
There was a problem hiding this comment.
That's right, it does. Yes, I'm curious about the impact on 262.
| { | ||
| ObjectSlotAttributes attr = objectAttrs ? objectAttrs[tmpIndex] : ObjectSlotAttr_Default; | ||
| if (!(attr & ObjectSlotAttr_Deleted) && attr != ObjectSlotAttr_Setter && | ||
| !!(flags & EnumeratorFlags::EnumNonEnumerable) || (attr & ObjectSlotAttr_Enumerable)) |
There was a problem hiding this comment.
Are we missing some parentheses here? It looks like we want to skip ObjectSlotAttr_Setter, but it would test as true for (attr & ObjectSlotAttr_Enumerable).
There was a problem hiding this comment.
I just noticed this line is also causing the compilation failure on xplat; it doesn't like expressions using both && and ||, for good reason.
There was a problem hiding this comment.
Yeah, I think you're right about the parens.
There was a problem hiding this comment.
&& binds more tightly than || so this would combine all the ANDs before doing the OR. Don't know whether that's correct but yeah, mixing AND and OR in the same expression without parens is asking for trouble. clang will throw a fit if it sees such an expression.
pleath
force-pushed
the
sharedeleted
branch
from
6b5dc17
to
6caf4a2
Compare
pleath
force-pushed
the
sharedeleted
branch
from
6caf4a2
to
4056962
Compare
|
@pleath this old PR of yours looks like a good change to me please could you tell me if you know why it wasn't merged? |
|
Yeah, I like this change. It was one of a number of improvements to type-sharing that were made in 2017-2018. I never fully tested it, though, and then the internal team stopped investing in future work. If you're serious about merging it, then I'd want to look back over the change to see if anything seems squirrelly with the benefit of hindsight. And it would at least have to go through a fuzzer and crawler run. |
|
@pleath thanks for the reply - unfortunately we weren't given access to the old MS fuzzers or crawler. We definitely need to get fuzzing set up if we're going to do much more work. I'll leave this until we have fuzzing available. |
Allow PathTypeHandler to support deleted properties. Also fix an existing issue involving enumeration order of deleted and re-added properties. (Dictionary type handlers still do this incorrectly.) This means we can no longer use DeleteProperty plus SetPropertyWithAttributes to handle converting accessor properties to data.