Skip to content

Fix #884: Proxy: ToPropertyDescriptor assert; when assert not hit, should throw TypeError.#885

Merged
dilijev merged 4 commits into
chakra-core:release/1.2from
dilijev:proxyassert
Apr 30, 2016
Merged

Fix #884: Proxy: ToPropertyDescriptor assert; when assert not hit, should throw TypeError.#885
dilijev merged 4 commits into
chakra-core:release/1.2from
dilijev:proxyassert

Conversation

@dilijev
Copy link
Copy Markdown
Contributor

@dilijev dilijev commented Apr 28, 2016

Fixes #884 (see issue for details).


This change is Reviewable

dilijev added 2 commits April 28, 2016 13:36
…ther the underlying object actually has a property (has() can be true, but get() can still fail to find a value).
… if it isn't there, set it to the default value. This causes a semantically correct TypeError to be thrown from ToPropertyDescriptor.
@dilijev dilijev added this to the 1.2 milestone Apr 28, 2016
@dilijev
Copy link
Copy Markdown
Contributor Author

dilijev commented Apr 28, 2016

@bterlson @digitalinfinity Review please? :)

@bterlson
Copy link
Copy Markdown
Contributor

👍

Comment thread lib/Parser/rterrors.h
RT_ERROR_MSG(JSERR_Property_NeedFunction, 5051, "The value of the property '%s' is not a Function object", "Function expected", kjstTypeError, JSERR_NeedFunction)
RT_ERROR_MSG(JSERR_Property_NeedFunction_NullOrUndefined, 5052, "The value of the property '%s' is null or undefined, not a Function object", "Function expected", kjstTypeError, JSERR_NeedObject)
RT_ERROR_MSG(JSERR_Property_CannotHaveAccessorsAndValue, 5053, "", "Property cannot have both accessors and a value", kjstTypeError, VBSERR_ActionNotSupported)
RT_ERROR_MSG(JSERR_Property_CannotHaveAccessorsAndValue, 5053, "", "Invalid property descriptor: cannot both specify accessors and a 'value' attribute", kjstTypeError, VBSERR_ActionNotSupported)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

cannot specify both

Copy link
Copy Markdown
Contributor Author

@dilijev dilijev Apr 29, 2016

Choose a reason for hiding this comment

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

I was worried about potential confusion here. The term "both" in "specify both accessors" could refer to the two accessors: "get" and "set". The grammar as written is technically correct and avoids that potential confusion.

Also this matches the grammar of v8's error message:
VM79:4 Uncaught TypeError: Invalid property descriptor. Cannot both specify accessors and a value or writable attribute, [object Object](…)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Huh, its like v8's implementer shared your same concern.

I don't think it is ambiguous because of the and but I'm only one sample point. It's fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll try to reword this to avoid both grammatical and semantic ambiguity. :)

@ianwjhalliday
Copy link
Copy Markdown
Collaborator

👍

@dilijev
Copy link
Copy Markdown
Contributor Author

dilijev commented Apr 29, 2016

@dotnet-bot test this please

@dilijev dilijev merged commit f4b45c5 into chakra-core:release/1.2 Apr 30, 2016
dilijev added a commit that referenced this pull request Apr 30, 2016
…t not hit, should throw TypeError.

Merge pull request #885 from dilijev:proxyassert
Fixes #884 (see issue for details).
@dilijev dilijev deleted the proxyassert branch April 30, 2016 00:02
dilijev added a commit that referenced this pull request Apr 30, 2016
…t; when assert not hit, should throw TypeError.

Merge pull request #885 from dilijev:proxyassert
Fixes #884 (see issue for details).
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.

4 participants