Eagerly check listing/mapping in iterables #752
Merged
Conversation
This file contains 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
holzensp
requested changes
Oct 30, 2024
There is currently a bug around resolving variables within the iterable of a for generator or spread syntax (apple#741) Normally, mappings/listings are type-checked lazily. However, this results in the said bug getting widened, for any object members declared in the iterable. As a workaround for now, prevent the bug from being any worse by ensuring that these object members are eagerly typechecked.
bioball
force-pushed
the
eager-check-iterables
branch
from
f705df9 to
c2f7879
Compare
|
Okay, this is now using the AstBuilder approach. This works a lot better; take another look! |
holzensp
approved these changes
Oct 31, 2024
| createSourceSection(ctx), | ||
| expr, | ||
| ctx.QSPREAD() != null, | ||
| symbolTable.getCurrentScope().isVisitingIterable()); |
There was a problem hiding this comment.
If this scope is not aligned with the above, something went wrong, no?
Suggested change
| symbolTable.getCurrentScope().isVisitingIterable()); | |
| visitingIterable); |
| @@ -27,6 +27,9 @@ | |||
| import org.pkl.core.util.Nullable; | |||
|
|
|||
| public final class ObjectMember extends Member { | |||
|
|
|||
| private final boolean isInIterable; | |||
There was a problem hiding this comment.
Seems costly... not worth rolling it into the member's modifiers? Probably worth profiling (both jpkl and native) on large corpora.
There was a problem hiding this comment.
Good idea! Will make this a modifier instead.
pkl-core/src/main/java/org/pkl/core/ast/member/PropertyTypeNode.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/ast/member/PropertyTypeNode.java
Outdated
Show resolved
Hide resolved
| @@ -26,6 +26,7 @@ | |||
| /** A property definition that has a type annotation. */ | |||
| public final class TypedPropertyNode extends RegularMemberNode { | |||
| @Child private DirectCallNode typeCheckCallNode; | |||
| private final ObjectMember member; | |||
There was a problem hiding this comment.
If booleans can be tightly packed, the cost concern in ObjectMember (above) goes away. In that case, I'd also just grab the single boolean here, because a pointer won't be.
| @@ -35,9 +38,11 @@ public ObjectMember( | |||
| SourceSection headerSection, | |||
| int modifiers, | |||
| @Nullable Identifier name, | |||
| String qualifiedName) { | |||
| String qualifiedName, | |||
| boolean isInIterable) { | |||
There was a problem hiding this comment.
There's quite a lot of extra code throughout, all with a default false case. I don't care much, considering it's likely rather temporary, but might it be worth an extra constructor that defaults this to false?
Co-authored-by: Philip K.F. Hölzenspies <[email protected]>
bioball
force-pushed
the
eager-check-iterables
branch
from
1d03f34 to
3e00f3f
Compare
bioball
force-pushed
the
eager-check-iterables
branch
from
76007bb to
aa38373
Compare
bioball
added a commit
to bioball/pkl
that referenced
this pull request
Nov 4, 2024
This mitigates an issue where lazy mappings and listings widen an existing bug. This is a follow-up to apple#752.
bioball
added a commit
to bioball/pkl
that referenced
this pull request
Nov 4, 2024
This mitigates an issue where lazy mappings and listings widen an existing bug. This is a follow-up to apple#752.
bioball
added a commit
that referenced
this pull request
Nov 5, 2024
This mitigates an issue where lazy mappings and listings widen an existing bug. This is a follow-up to #752.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When we introduced lazy listing/mapping typechecking, we widened an existing bug around resolving for-generator variables in #741.
To prevent this bug from being widened, this introduces logic to eagerly evaluate any
Listing<V>orMapping<K, V>found in the iterable.We're not sure what the best approach is yet to fix the bug. In the meantime, this prevents the existing bug from being any more prevalent than it currently is.