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

Implement Vector.build: implementation and some tests #9725

Merged
merged 13 commits into from
Apr 18, 2024

Conversation

GregoryTravis
Copy link
Contributor

@GregoryTravis GregoryTravis commented Apr 16, 2024

This PR implements Vector.build and converts uses of .new_builder to .build within Vector and Vector_Spec.

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.

@GregoryTravis GregoryTravis marked this pull request as ready for review April 16, 2024 20:13
@@ -171,6 +174,50 @@ type Vector a
new_builder : Integer -> Builder
new_builder (capacity=10) = Builder.new capacity

## PRIVATE
ADVANCED
Creates a new `Vector` by providing a `Builder` to the provided function.
Copy link
Member

Choose a reason for hiding this comment

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

I'd rephrase this sentence as providing X to the provided Y sounds weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to "passing a Builder to the provided function"

builder.append 3
# => [1, 2, 3]
build : (Builder -> Any) -> Integer -> Vector
build (function : Builder -> Any) (capacity : Integer = 10) -> Vector =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
build (function : Builder -> Any) (capacity : Integer = 10) -> Vector =
build (function : Builder -> Any) (initial_capacity : Integer = 10) -> Vector =

I think maybe adding initial_ would clarify the meaning of this variable; without initial it could sound like it's the max capacity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +1046 to +1051
group_builder.specify "Should propagate dataflow errors" <|
v = Vector.build builder->
builder.append 1
builder.append 2
Error.throw (Illegal_Argument.Error "asdf")
v . should_fail_with (Illegal_Argument.Error "asdf")
Copy link
Member

Choose a reason for hiding this comment

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

This is a great test.

Not directly related, but while we are at it we could consider solving a long outstanding problem we used to have:

        group_builder.specify "Should propagate the first dataflow error encountered in `append`" <|
            v = Vector.build builder->
                builder.append 1
                builder.append (Error.throw (Illegal_Argument.Error "ERROR2")
                builder.append 3
                builder.append (Error.throw (Illegal_Argument.Error "ERROR4")
                builder.append 5
            v . should_fail_with (Illegal_Argument.Error "ERROR2")

Unfortunately, as far as I know currently this will actually not fail but produce a vector [1, 3, 5], essentially swallowing the dataflow errors.

We used to have a pattern of 'folding' the builder i.e. append returns a builder and this would have to be rewritten as:

                builder . append 1
                        . append (Error.throw (Illegal_Argument.Error "ERROR2")
                        . append 3
                        . append (Error.throw (Illegal_Argument.Error "ERROR4")
                        . append 5

in which case the error would be correctly propagated.

Unfortunately, the 'folding the builder' pattern did not get much traction. In many places it was just not too practical - the builder was inherently mutable object so this more 'functional' folding was often not fitting its usages.

Do you think we could amend the builder to detect that append got a dataflow error and avoid swallowing it?

This could be implemented by holding a encountered_error : Ref (Any | Nothing) inside of Builder, then append can check item.is_error and set this Ref the first time an error is encountered; then Builder.to_vector would first check if encountered_error is set and if so, propagate that error instead of building the vector.

Copy link
Member

Choose a reason for hiding this comment

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

But this is a bit out of scope of this PR, so I guess we can also do it as a separate ticket/PR instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good idea. #9733

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.

Looks great.

Your test inspired me to try improving a dataflow-error swallowing issue with Builder.append - but I guess that may be a task for a separate PR.

@GregoryTravis GregoryTravis requested review from radeusgd and JaroslavTulach and removed request for radeusgd April 18, 2024 20:34
@GregoryTravis GregoryTravis merged commit 681df82 into develop Apr 18, 2024
36 checks passed
@GregoryTravis GregoryTravis deleted the wip/gmt/9304-vector-build branch April 18, 2024 21:52
b.append x
b.append (State.get Number).to_text
b.to_vector
Vector.build builder->
Copy link
Member

Choose a reason for hiding this comment

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

I'd assume this is a general even for complicated computations. Just wrap your computation in Vector.build builder-> block and that's it.

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.

4 participants