Skip to content
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

Share types with deleted properties #5704

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pleath
Copy link
Contributor

@pleath pleath commented Sep 14, 2018

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.

delete obj['a']
TEST('{"c":3,"b":2,"d":4}', JSON.stringify(obj));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed this line is also causing the compilation failure on xplat; it doesn't like expressions using both && and ||, for good reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think you're right about the parens.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& 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.

Copy link
Contributor

@sethbrenith sethbrenith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jan 5, 2021

@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?

@pleath
Copy link
Contributor Author

pleath commented Jan 5, 2021

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.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jan 6, 2021

@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.

@rhuanjl rhuanjl mentioned this pull request Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants