Respect fixed and const properties when converting Map or Dynamic to Typed - #1814
Open
nileshpatil6 wants to merge 2 commits into
Open
Respect fixed and const properties when converting Map or Dynamic to Typed#1814nileshpatil6 wants to merge 2 commits into
nileshpatil6 wants to merge 2 commits into
Conversation
toTyped() on Map and Dynamic built its delegating members the same way for every property, including ones marked fixed. That let a caller supply any value for a fixed property through the source Map or Dynamic, silently overriding the value fixed by the class declaration. createDelegatingMembers is now told whether it is building members for a conversion into Typed. When that is the case, fixed properties are skipped just like hidden properties already are, so the class prototype's own fixed value is used instead of whatever is in the source Map or Dynamic. The Typed to Dynamic direction is unaffected since it already reads from the evaluated Typed value. Added snippet tests in map.pkl and dynamic.pkl covering toTyped() on a class with a fixed property. Fixes apple#573 Signed-off-by: nileshpatil6 <technil6436@gmail.com>
const properties have the same override-immunity requirement as fixed properties: a value supplied by the source Map/Dynamic must not replace the class's own declared value. Change the isFixed() check in createDelegatingMembers to isConstOrFixed(), and add matching test cases alongside the existing fixed ones in map.pkl and dynamic.pkl. Signed-off-by: nileshpatil6 <technil6436@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Reopens #1733. I accidentally closed that PR permanently by deleting my fork while cleaning up old repos, sorry for the churn. Same two commits, rebased onto current main (VmClass.java had not changed upstream, so the rebase was clean).
Recap of where the review stood:
.toTyped()copied whatever value the source Dynamic/Map happened to contain intofixedproperties, silently overriding the class declaration. Fixed by skipping such properties during conversion inVmClass.createDelegatingMembers, so the class's own value wins.constproperties. The second commit switches the check to the existingMember.isConstOrFixed(), matching howGeneratorMemberNodeandSpecializedObjectLiteralNodealready treat const and fixed as one category for override protection. Snippet tests cover both cases inmap.pklanddynamic.pkl.Re-verified after the rebase:
./gradlew :pkl-core:test --tests "*LanguageSnippetTests*"passes, the api input dir reports 158 tests, 0 failures.DCO sign-off is on both commits.