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

Add Vector.build_multiple and handle dataflow error and warning propagation in build and build_multiple #9766

Merged
merged 98 commits into from
May 7, 2024

Conversation

GregoryTravis
Copy link
Contributor

@GregoryTravis GregoryTravis commented Apr 22, 2024

Adds Vector.build_multiple to build multiple Vectors at the same time.
Also handles uncaught dataflow errors being added to builders or returned from builder callbacks, as well as attached warning propagation.
This converts the remaining uses of Vector.new_builder to Vector.build in Standard.Base.

Following @Jaroslav’s suggestion, this PR implements the propagation of dataflow errors passed to Builder.append by wrapping them in a Panic.

This is implemented as Panic.throw_wrapped_if_error and Panic.handle_wrapped_dataflow_error, which I have modified to allow specifying the error type and a custom handler.

Screen Shot 2024-04-26 at 2 53 52 PM

Previously, we relied on the dataflow errors being propagated by Builder.append, which sometimes required the fold pattern to ensure that error values were not added to the Builder as if they were regular values.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Reconciling the very functional dataflow error and warning mechanisms with these rather-imperative builders is quite a challenge. I'm really really happy we are patching the holes and making the error/warning propagation more robust - that's really great!

I've got some minor comments. The biggest issue I see is the quadratic complexity explosion at warnings handling.

Maybe we should consider changing our underlying data structures, to make + on vectors more efficient (we could use a similar Rope structure as we use on the Text type). But that's a bigger effort. For now let's just (slightly ironically) use an imperative builder there.

@GregoryTravis GregoryTravis added CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge labels May 1, 2024
self.warnings.put (self.warnings.get + (warnings.map .value))
Nothing
warnings = Warning.get_all x . map .value
append_result = self.warnings_java_builder.appendTo (warnings . slice 0 warnings.length)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this slice needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this was needed to prevent an error, but it's not happening now, so I've removed it.

@mergify mergify bot merged commit a2c36de into develop May 7, 2024
34 checks passed
@mergify mergify bot deleted the wip/gmt/9304-tricky branch May 7, 2024 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants