avm2: Return undefined for construct_prop on primitives and namespaces #21417
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danielhjacobs
added
A-avm2
SuchAFuriousDeath
force-pushed
the
primitive-construct-prop
branch
from
6f40a49 to
437a156
Compare
SuchAFuriousDeath
force-pushed
the
primitive-construct-prop
branch
from
437a156 to
9ab1f72
Compare
| } | ||
| _ => self | ||
| .get_property(multiname, activation) | ||
| .unwrap_or_else(|_| { |
There was a problem hiding this comment.
This still feels like a hacky solution to me, since
- The property not being found is not the only reason
get_propertycan return anErr - avmplus's version of
construct_propertythrows different errors than itsget_property(e.g. when run on a slot that isn't callable), so we'll have to change it to work that way when we implement it correctly anyways
Also, this shouldn't be trying to construct the proto; it should be trying to construct the result of a dynamic lookup of the multiname on the proto.
Could you make this match the way avmplus does it, or even just inline the vtable lookup logic from get_property here (with the special case for primitives)? That would solve the issues I mentioned above.
Lord-McSweeney
added
waiting-on-author
SuchAFuriousDeath
force-pushed
the
primitive-construct-prop
branch
from
9ab1f72 to
e28e617
Compare
SuchAFuriousDeath
force-pushed
the
primitive-construct-prop
branch
from
e28e617 to
286673f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.