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 documentation for select #747

Merged

Conversation

straight-shoota
Copy link
Member

No description provided.

Copy link

netlify bot commented Jan 29, 2024

Deploy Preview for crystal-book ready!

Name Link
🔨 Latest commit 2dc5c68
🔍 Latest deploy log https://app.netlify.com/sites/crystal-book/deploys/65b824246d5aea0008162fa5
😎 Deploy Preview https://deploy-preview-747--crystal-book.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@straight-shoota straight-shoota changed the base branch from master to release/1.11 January 29, 2024 22:21
@straight-shoota straight-shoota linked an issue Jan 29, 2024 that may be closed by this pull request
Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Are you sure we should document the internal implementation details until we move on to a public API? For the time being I'd document sugar syntax (the supported expressions) and forego the details of the syntax expansion (internal detail).

BTW: I'm surprised that #timeout_select_action is documented, it even returns a Channel::TimeoutAction that isn't documented.

Also: we can select on Channel#send(M).

@straight-shoota
Copy link
Member Author

Well this documents the current state. If we change the API, we'll have to change the documentation. But at least we'll have something to start with. AFAIK the _select_action expansion is intended to be used for custom extensions. It just doesn't work that well right now.
And frankly, I wouldn't expect the API to change that much, except that the types become public, probably in the Crystal namespace. Details are to be determined, of course.

@straight-shoota
Copy link
Member Author

Also: we can select on Channel#send(M).

Not sure what you mean by that... Could you clarify?

@ysbaddaden
Copy link
Contributor

I agree the syntax expansion shouldn't change, but we never know and we can't use the knowledge anyway (because private classes) 🤷

I mean that we can select on send too, but I see you did list Channel#send_select_action. There is no example, though:

select
when channel.send(message)
when timeout(5.seconds)
end

@straight-shoota
Copy link
Member Author

straight-shoota commented Feb 6, 2024

Yeah, we'll need a guide on how to use select with standard select actions. This should be a separate document though, and not part of the language specification.

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

I always come back to https://gist.github.com/bcardiff/289953a80eb3a0512a2a2f8c8dfeb1db to see the potential uses of select and it's variation.

That guide might serve better for a guide that covers the use cases rather than the language semantics.

Maybe for the language semantics, if we want to be precise we should mention the conversion of select statements to Channel.select/Channel.non_blocking_select and refer to that (future) guide for further uses.

The proposed document is short and concise, so if we iterate on the comments I think is a good addition. Definitely the usage guide will be well received.

@@ -0,0 +1,52 @@
# select

The `select` expression chooses from a set of blocking operations and proceeds with the branch that becomes available first.
Copy link
Member

Choose a reason for hiding this comment

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

Available first is not 100% accurate in MT or even if both are already available. I'm not sure how to frame the difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

If any actions are directly executable, select_impl immediately returns the result of the first action with delivery state delivered or closed. So IIUC "available first" should be good for that scenario.

With multithreading everything is a bit more complicated, of course. But even there the action that first manages to activate the select context is chosen.

Maybe we just need a different word for "available"?

Suggested change
The `select` expression chooses from a set of blocking operations and proceeds with the branch that becomes available first.
The `select` expression chooses from a set of blocking operations and proceeds with the branch that activates first.


## Select actions

A select action call calls a method with the implicit suffix "_select_action".
Copy link
Member

Choose a reason for hiding this comment

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

The ? suffix means the transformation is foo? -> foo_select_action?, and foo -> foo_select_action 🕵️ .

I don't remember if foo and foo_select_action are constrained to have some relation in their type signature.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a mention of the suffix.

I don't remember if foo and foo_select_action are constrained to have some relation in their type signature.

I don't think so. There's no need for any constraints because all calls in a select expression's where condition are expanded. If a method signature doesn't match, the type checker will complain.

Copy link
Member

Choose a reason for hiding this comment

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

They do.

def foo_select_action : Channel::TimeoutAction
  timeout_select_action(1.second)
end

a =
  select
  when x = foo
    x
  else
    nil
  end

complains with

In foo.ign.cr:15:12

 15 | when x = foo
               ^--
Error: undefined local variable or method 'foo' for top-level

If we define foo with the wrong type

def foo : Int32
  0
end

we get

 14 | select
      ^
Error: can't cast (Channel::NotReady | Nil) to Int32

Since Channel::TimeoutAction includes SelectAction(Nil) we need def foo : Nil for things to compile

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, interesting. I have no idea where in the compiler that would be happening, though 😆
Literal expander just transforms the calls without any checking.

docs/syntax_and_semantics/select.md Show resolved Hide resolved
@straight-shoota straight-shoota merged commit 590df79 into crystal-lang:release/1.11 Mar 22, 2024
2 checks passed
@straight-shoota straight-shoota deleted the feat/select branch March 22, 2024 12:15
@straight-shoota straight-shoota restored the feat/select branch March 22, 2024 12:15
@straight-shoota straight-shoota deleted the feat/select branch March 22, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document select keyword
3 participants