Skip to content
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

Fix globbed read/import bugs #449

Merged
merged 8 commits into from
Jun 4, 2024
Merged

Fix globbed read/import bugs #449

merged 8 commits into from
Jun 4, 2024

Conversation

translatenix
Copy link
Contributor

@translatenix translatenix commented Apr 24, 2024

This PR fixes several bugs related to globbed reads/imports and makes code improvements along the way.

Bug fixes:

  • Fix caching of globbed reads
    • Glob pattern isn't guaranteed to be a constant
    • Both glob resolution and ResolvedGlobElement.getPath() depend on the current module URI
  • Fix ClassCastException when reflecting on globbed import
  • Avoid creating an unbounded number of Truffle root nodes per globbed read/import

See commit messages for further details.

Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

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

Overall, great work!!

I have some minor blocking comments, but overall this is a great improvement. Thanks so much for your PR.

pkl-core/src/main/java/org/pkl/core/runtime/VmMap.java Outdated Show resolved Hide resolved
VmUtils.createEmptyMaterializedFrame(),
BaseModule.getMappingClass().getPrototype(),
members);
return self.toMapping();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should change class toDynamic to also call out to self.toDynamic() yeah?

@bioball
Copy link
Contributor

bioball commented May 20, 2024

Hi @translatenix, are you still planning on working on this PR, and the other ones? If not, we'd be happy to take it past the finish line (and credit you, of course)

translatenix and others added 8 commits May 28, 2024 13:19
SharedMemberNode enables generating non-constant object members
at run time without generating an unbounded number of Truffle root nodes.
Introduce VmObjectBuilder, a uniform way to build `VmObject`s
whose `ObjectMember`s are determined at run time.
Replace some manual object building code with VmObjectBuilder.
Add some assertions to fix IntelliJ warnings.
- Leverage SharedMemberNode to have a single Truffle root node per globbed
  read/import expression instead of one root node per resolved glob element.
- Remove caching in ReadGlobNode because it only works correctly if glob pattern is a string *constant*.
- Remove caching in StaticReadNode (now ReadGlobElementNode/ImportGlobElementNode)
  because it seems unnecessary and the implementation doesn't look quite right.
Problem:
The result of a globbed read depends not only on the glob pattern but also on the current module URI.
However, ResourceManager does not take this into account when caching globbed reads, causing wrong results.

Changes:
- Cache globbed reads per read expression.
  This is also how globbed imports are cached, except that import URIs are constant.
- Make ReadGlobNode and ImportGlobNode code as similar as possible.
- Reduce code duplication by inheriting from AbstractReadNode.
- Add some tests.
* Defer initialization of shared member node, add @child annotations
* Reduce footprint of truffle boundaries
* Revert bugfix when reflecting upon modules with globbed imports
@bioball
Copy link
Contributor

bioball commented May 28, 2024

FYI @translatenix given the lack of activity here, I'll go ahead and update this PR with my comments, and do the same for #455, #378 and #197. Thanks again for your contributions!

Copy link
Contributor

@stackoverflow stackoverflow left a comment

Choose a reason for hiding this comment

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

LGTM

@bioball bioball merged commit 207d0c7 into apple:main Jun 4, 2024
4 checks passed
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.

3 participants