-
Notifications
You must be signed in to change notification settings - Fork 280
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
Semantics of Listing typechecks have changed in 0.27 #785
Labels
bug
Something isn't working
Milestone
Comments
Ah, I think this is a regression in our typechecking logic. It shouldn't be possible to amend |
odenix
added a commit
to odenix/pkl
that referenced
this issue
Nov 7, 2024
Motivation: To implement lazy type checking of listings/mappings, major modifications were made to VmListingOrMapping and its subclasses. I believe a simpler implementation is possible and desirable. Changes: - Implement listing/mapping type cast via amending instead of delegation. This is the key idea. - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - remove overrides of VmObject methods that are no longer required - having a single implementation simplifies code and helps JITs - restore the invariant that a shallow-forced object has a fully populated cachedValues map - this invariant can be exploited to speed up iteration (not part of this PR) Result: - simpler code that will be easier to optimize - For example, restoring the invariant that a shallow-forced object has a fully populated cachedValues map can be exploited to speed up iteration (not part of this PR). - smaller memory footprint - better performance in some cases - fixes apple#785 Examples for potential future optimizations: - avoid type check overhead for untyped listings/mappings - make forcing a typed listing/mapping more efficient - currently, the parent chain is traversed once per member for type checking, which nullifies much of the performance advantage that shallow-forcing an object has over computing each value individually - avoid creating an intermediate untyped listing/mapping in the following cases: - new Listing<X> {...} - `property: Listing<X>` is amended
odenix
added a commit
to odenix/pkl
that referenced
this issue
Nov 7, 2024
Motivation: To implement lazy type checking of listings/mappings, major modifications were made to VmListingOrMapping and its subclasses. I believe a simpler implementation is possible and desirable. Changes: - Implement listing/mapping type cast via amending instead of delegation. This is the key idea. - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - remove overrides of VmObject methods that are no longer required - having a single implementation simplifies code and helps JITs Result: - simpler code that will be easier to optimize - For example, restoring the invariant that a shallow-forced object has a fully populated cachedValues map can be exploited to speed up iteration (not part of this PR). - smaller memory footprint - better performance in some cases - fixes apple#785 Examples for potential future optimizations: - avoid type check overhead for untyped listings/mappings - make forcing a typed listing/mapping more efficient - currently, the parent chain is traversed once per member for type checking, which nullifies much of the performance advantage that shallow-forcing an object has over computing each value individually - avoid creating an intermediate untyped listing/mapping in the following cases: - new Listing<X> {...} - `property: Listing<X>` is amended
odenix
added a commit
to odenix/pkl
that referenced
this issue
Nov 7, 2024
Motivation: As part of implementing lazy type checking of listings and mappings, major modifications were made to VmListingOrMapping and its subclasses. I believe a simpler implementation is possible and desirable. Changes: - Implement listing/mapping type cast via amendment (`parent`) instead of delegation (`delegate`). This is the key idea. - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - remove overrides of VmObject methods that are no longer required - having a single implementation simplifies code and helps JITs Result: - simpler code that will be easier to optimize - For example, restoring the invariant that a shallow-forced object has a fully populated cachedValues map can be exploited to speed up iteration (not part of this PR). - smaller memory footprint - better performance in some cases - fixes apple#785 Examples for potential future optimizations: - avoid type check overhead for untyped listings/mappings - make forcing a typed listing/mapping more efficient - currently, the parent chain is traversed once per member for type checking, which nullifies much of the performance advantage that shallow-forcing an object has over computing each value individually - avoid creating an intermediate untyped listing/mapping in the following cases: - new Listing<X> {...} - `property: Listing<X>` is amended
odenix
added a commit
to odenix/pkl
that referenced
this issue
Nov 14, 2024
Motivation: As part of implementing lazy type checking of listings and mappings, major modifications were made to VmListingOrMapping and its subclasses. I believe a simpler implementation is possible and desirable. Changes: - Implement listing/mapping type cast via amendment (`parent`) instead of delegation (`delegate`). This is the key idea. - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - remove overrides of VmObject methods that are no longer required - having a single implementation simplifies code and helps JITs Result: - simpler code that will be easier to optimize - For example, restoring the invariant that a shallow-forced object has a fully populated cachedValues map can be exploited to speed up iteration (not part of this PR). - smaller memory footprint - better performance in some cases - fixes apple#785 Examples for potential future optimizations: - avoid type check overhead for untyped listings/mappings - make forcing a typed listing/mapping more efficient - currently, the parent chain is traversed once per member for type checking, which nullifies much of the performance advantage that shallow-forcing an object has over computing each value individually - avoid creating an intermediate untyped listing/mapping in the following cases: - new Listing<X> {...} - `property: Listing<X>` is amended
odenix
added a commit
to odenix/pkl
that referenced
this issue
Nov 28, 2024
Motivation: As part of implementing lazy type checking of listings and mappings, major modifications were made to VmListingOrMapping and its subclasses. I believe a simpler implementation is possible and desirable. Changes: - Implement listing/mapping type cast via amendment (`parent`) instead of delegation (`delegate`). This is the key idea. - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - remove overrides of VmObject methods that are no longer required - having a single implementation simplifies code and helps JITs Result: - simpler code that will be easier to optimize - For example, restoring the invariant that a shallow-forced object has a fully populated cachedValues map can be exploited to speed up iteration (not part of this PR). - smaller memory footprint - better performance in some cases - fixes apple#785 Examples for potential future optimizations: - avoid type check overhead for untyped listings/mappings - make forcing a typed listing/mapping more efficient - currently, the parent chain is traversed once per member for type checking, which nullifies much of the performance advantage that shallow-forcing an object has over computing each value individually - avoid creating an intermediate untyped listing/mapping in the following cases: - new Listing<X> {...} - `property: Listing<X>` is amended
odenix
added a commit
to odenix/pkl
that referenced
this issue
Dec 4, 2024
Motivation: - simplify implementation of lazy type checking - fix correctness issues of lazy type checking (apple#785) Changes: - implement listing/mapping type cast via amendment (`parent`) instead of delegation (`delegate`) - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - fix apple#785 by executing all type casts between a member's owner and receiver - fix apple#823 by storing owner and receiver directly instead of storing the mutable frame containing them (typeNodeFrame) - remove overrides of VmObject methods that are no longer required - good for Truffle partial evaluation and JVM inlining - revert a85a173 except for added tests - move `VmUtils.setOwner` and `VmUtils.setReceiver` and make them private - these methods aren't generally safe to use Result: - simpler code with greater optimization potential - VmListingOrMapping can now have both a type node and new members - fewer changes to surrounding code - smaller memory footprint - better performance in some cases - fixes apple#785 - fixes apple#823 Potential future optimizations: - avoid lazy type checking overhead for untyped listings/mappings - improve efficiency of forcing a typed listing/mapping - currently, lazy type checking will traverse the parent chain once per member, reducing the performance benefit of shallow-forcing a listing/mapping over evaluating each member individually - avoid creating an intermediate untyped listing/mapping in the following cases: - `new Listing<X> {...}` - amendment of `property: Listing<X>`
odenix
added a commit
to odenix/pkl
that referenced
this issue
Dec 4, 2024
Motivation: - simplify implementation of lazy type checking - fix correctness issues of lazy type checking (apple#785) Changes: - implement listing/mapping type cast via amendment (`parent`) instead of delegation (`delegate`) - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - fix apple#785 by executing all type casts between a member's owner and receiver - fix apple#823 by storing owner and receiver directly instead of storing the mutable frame containing them (typeNodeFrame) - remove overrides of VmObject methods that are no longer required - good for Truffle partial evaluation and JVM inlining - revert a85a173 except for added tests - move `VmUtils.setOwner` and `VmUtils.setReceiver` and make them private - these methods aren't generally safe to use Result: - simpler code with greater optimization potential - VmListingOrMapping can now have both a type node and new members - fewer changes to surrounding code - smaller memory footprint - better performance in some cases - fixes apple#785 - fixes apple#823 Potential future optimizations: - avoid lazy type checking overhead for untyped listings/mappings - improve efficiency of forcing a typed listing/mapping - currently, lazy type checking will traverse the parent chain once per member, reducing the performance benefit of shallow-forcing a listing/mapping over evaluating each member individually - avoid creating an intermediate untyped listing/mapping in the following cases: - `new Listing<X> {...}` - amendment of `property: Listing<X>`
stackoverflow
pushed a commit
that referenced
this issue
Dec 6, 2024
Motivation: - simplify implementation of lazy type checking - fix correctness issues of lazy type checking (#785) Changes: - implement listing/mapping type cast via amendment (`parent`) instead of delegation (`delegate`) - handle type checking of *computed* elements/entries in the same way as type checking of computed properties - ElementOrEntryNode is the equivalent of TypeCheckedPropertyNode - remove fields VmListingOrMapping.delegate/typeNodeFrame/cachedMembers/checkedMembers - fix #785 by executing all type casts between a member's owner and receiver - fix #823 by storing owner and receiver directly instead of storing the mutable frame containing them (typeNodeFrame) - remove overrides of VmObject methods that are no longer required - good for Truffle partial evaluation and JVM inlining - revert a85a173 except for added tests - move `VmUtils.setOwner` and `VmUtils.setReceiver` and make them private - these methods aren't generally safe to use Result: - simpler code with greater optimization potential - VmListingOrMapping can now have both a type node and new members - fewer changes to surrounding code - smaller memory footprint - better performance in some cases - fixes #785 - fixes #823 Potential future optimizations: - avoid lazy type checking overhead for untyped listings/mappings - improve efficiency of forcing a typed listing/mapping - currently, lazy type checking will traverse the parent chain once per member, reducing the performance benefit of shallow-forcing a listing/mapping over evaluating each member individually - avoid creating an intermediate untyped listing/mapping in the following cases: - `new Listing<X> {...}` - amendment of `property: Listing<X>`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The following program fails in 0.26.3 but passes in 0.27.0.
(My understanding is that the goal was to preserve semantics of Listing typechecks.)
The text was updated successfully, but these errors were encountered: