Carrier type for Choose and Offer; lift Choice<N> restriction #98

sdleffler · 2021-04-08T17:16:40Z

No description provided.

plaidfinch

Finally reviewing through all this: overall, looks great! Clean implementation, clear documentation. Well done :)

My only large change request is: it seems preferable to have the Carrier type be the second argument to Choose and Offer, with a defaulted type, so that error messages containing the usual Choose and Offer types with Choices don't get more cluttered, and code that does not use the Session! macro is backwards-compatible.

In order to achieve this defaulting, we'd need a trait to get the Choice<N> for a given tuple's arity, so, e.g. Offer can be Offer<Choices, <ChoiceForArity<Choices>>::Choice> or some such. What are your thoughts about this change?

There are also some conflicts with main now, largely due to changes to the definitions of Offer and Choose so as to make their constructors private.

sdleffler · 2021-06-02T18:05:41Z

Finally reviewing through all this: overall, looks great! Clean implementation, clear documentation. Well done :)

My only large change request is: it seems preferable to have the Carrier type be the second argument to Choose and Offer, with a defaulted type, so that error messages containing the usual Choose and Offer types with Choices don't get more cluttered, and code that does not use the Session! macro is backwards-compatible.

In order to achieve this defaulting, we'd need a trait to get the Choice<N> for a given tuple's arity, so, e.g. Offer can be Offer<Choices, Carrier = <Choices as ChoiceForArity>::Choice> or some such. What are your thoughts about this change?

There are also some conflicts with main now, largely due to changes to the definitions of Offer and Choose so as to make their constructors private.

I don't see an issue with that kind of defaulting, as long as Rust doesn't get confused by having a default type parameter depend on a non-default one. There will have to be an obvious limit on the maximum tuple arity for such.

…d build untested

plaidfinch · 2021-10-20T15:43:35Z

This also constitutes a breaking API change to Dialectic, so should be a minor version bump (since we're pre-1.0).

plaidfinch · 2021-10-20T15:48:08Z

To implement the changes suggested above, there are several steps:

1. Swap the order of the type parameters in Choose and Offer types, and fix all type errors that result. (Done in Swap Offer and Choose generic parameter order. #122)
2. Change the definition of the Session! macro so that it emits those type parameters in the correct order.
3. Define the ChoiceForArity trait to compute the default Choice for a given tuple of some particular arity. This should be defined inductively over the type-level list representation of the tuple, rather than directly as a trait on the tuple. This avoids needing to write a giant number of instances!
4. Add that trait's associated type as a default type parameter value to Choose and Offer to compute the appropriate default Choice<N> for each use site of the type.
5. Modify the Session! macro so that it omits generating explicit carrier types as a second parameter when the carrier type is default (this will mean that cargo expand will not contain them either).
6. Remove the value parameter from the choose method of Chan, reverting it a generalization of its earlier signature, and making it always send () as a variant payload. This generalizes over the previous definition by making choose applicable to any variant which does not have any information to be sent with it.
7. Add a new method to Chan called choose_with, which is defined exactly as the current (in this PR) definition of choose, taking a value that is to be sent as a payload with the choice.

yaymukund · 2021-11-02T00:38:27Z

It also looks like docs + clippy fail on this PR.

warning: lint `broken_intra_doc_links` has been renamed to `rustdoc::broken_intra_doc_links`
   --> dialectic-compiler/src/lib.rs:253:11
    |
253 | #![forbid(broken_intra_doc_links)]
    |           ^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `rustdoc::broken_intra_doc_links`
    |
    = note: `#[warn(renamed_and_removed_lints)]` on by default

warning: `dialectic-compiler` (lib doc) generated 1 warning
warning: lint `broken_intra_doc_links` has been renamed to `rustdoc::broken_intra_doc_links`
 --> dialectic-macro/src/lib.rs:1:11
  |
1 | #![forbid(broken_intra_doc_links)]
  |           ^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `rustdoc::broken_intra_doc_links`
  |
  = note: `#[warn(renamed_and_removed_lints)]` on by default

warning: `dialectic-macro` (lib doc) generated 1 warning
 Documenting dialectic v0.4.1 (/home/mukund/src/dialectic/dialectic)
warning: lint `broken_intra_doc_links` has been renamed to `rustdoc::broken_intra_doc_links`
   --> dialectic/src/lib.rs:133:11
    |
133 | #![forbid(broken_intra_doc_links)]
    |           ^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `rustdoc::broken_intra_doc_links`
    |
    = note: `#[warn(renamed_and_removed_lints)]` on by default

error: unknown disambiguator `crate::Session`
   --> dialectic/src/lib.rs:87:51                                                                                        
    |
87  | - To specify a session type, use the [`Session!`](crate::Session@macro) macro, which defines a
    |                                                   ^^^^^^^^^^^^^^
    |
note: the lint level is defined here
   --> dialectic/src/lib.rs:133:11
    |
133 | #![forbid(broken_intra_doc_links)]
    |           ^^^^^^^^^^^^^^^^^^^^^^
    = note: see https://doc.rust-lang.org/1.56.0/rustdoc/linking-to-items-by-name.html#namespaces-and-disambiguators for more info about disambiguators

error: unknown disambiguator `crate::Session`
  --> dialectic/src/lib.rs:90:77
   |
90 | - To construct a session-typed [`Chan`], use the methods on the [`Session`](crate::Session@trait)
   |                                                                             ^^^^^^^^^^^^^^
   |
   = note: see https://doc.rust-lang.org/1.56.0/rustdoc/linking-to-items-by-name.html#namespaces-and-disambiguators for more info about disambiguators

error: unknown disambiguator `crate::Session`
   --> dialectic/src/lib.rs:103:16
    |
103 | | [`Session!`](crate::Session@macro) Macro Invocation | Session Type (`S`) | Channel Operation(s)<br>(on a channel `c: Chan<S, _, _>`) | ...
    |                ^^^^^^^^^^^^^^
    |
    = note: see https://doc.rust-lang.org/1.56.0/rustdoc/linking-to-items-by-name.html#namespaces-and-disambiguators for more info about disambiguators

error: unknown disambiguator `crate::Session`
  --> dialectic/src/session.rs:51:21
   |
51 | ///    [`Session!`](crate::Session@macro) macro, because it will throw an error if you try.
   |                     ^^^^^^^^^^^^^^
   |
   = note: see https://doc.rust-lang.org/1.56.0/rustdoc/linking-to-items-by-name.html#namespaces-and-disambiguators for more info about disambiguators

warning: `dialectic` (lib doc) generated 1 warning
error: could not document `dialectic`

Caused by:
<snip>

I'm on rustc & cargo 1.56, in case it's version-specific.

sdleffler marked this pull request as ready for review April 8, 2021 23:18

sdleffler requested a review from plaidfinch April 8, 2021 23:18

This was linked to issues Apr 13, 2021

[transmit-case] Issues with TransmitCase by ref #99

Open

[transmit-case] Ergonomics for Choice<N> bounds #100

Open

plaidfinch mentioned this pull request May 19, 2021

Add small "real-world" protocol specification/example to the tutorial #61

Open

plaidfinch suggested changes Jun 2, 2021

View reviewed changes

sdleffler and others added 18 commits June 2, 2021 11:20

Checkpoint: dialectic/dialectic-macro/dialectic-compiler typecheck an…

e5d035c

…d build untested

Checkpoint: everything compiles without blowing up

94d6013

Use crates.io vesta; start fixing tests

097f0fe

More test fixes

386f224

Fix a few test cases by correcting compile errors

5a70add

Fix missing () in macro doctest

a9cef7e

Fix nasty little bug friend

5900112

Fix arbitrary instance for Syntax

8a8f2fb

Documentation and harmlessly missing Match bound

03748bf

Sate the eternal hunger of Clippy

0e35c8f

Fix broken doc link

bf5546a

As per discussion of issues #99 and #100

9ae6a6d

Fix call-by version

0b4c21b

Doc changes and add tutorial section w/ contrived example

b145717

call-by update

cdae139

Polish tutorial docs for custom carrier types a bit

dab31eb

Polish docs for backend

4fd8a4e

Fix broken doc link

ddd8132

sdleffler force-pushed the carrier-type branch from 3a547cd to ddd8132 Compare June 2, 2021 18:21

sdleffler added 2 commits June 2, 2021 11:32

Adjust dialectic-reconnect to use TransmitChoice

0b5c002

Adjust dialectic-reconnect to work w/ carrier-type.

3edf3c6

plaidfinch linked an issue Jun 8, 2021 that may be closed by this pull request

offer! macro does not catch too-small arity #115

Open

Swap Offer and Choose generic parameter order. (#122)

6b5f6ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Carrier type for Choose and Offer; lift Choice<N> restriction #98

Carrier type for Choose and Offer; lift Choice<N> restriction #98

sdleffler commented Apr 8, 2021

plaidfinch left a comment •

edited

Loading

sdleffler commented Jun 2, 2021 •

edited by plaidfinch

Loading

plaidfinch commented Oct 20, 2021

plaidfinch commented Oct 20, 2021 •

edited by yaymukund

Loading

yaymukund commented Nov 2, 2021

Carrier type for Choose and Offer; lift Choice<N> restriction #98

Are you sure you want to change the base?

Carrier type for Choose and Offer; lift Choice<N> restriction #98

Conversation

sdleffler commented Apr 8, 2021

plaidfinch left a comment • edited Loading

Choose a reason for hiding this comment

sdleffler commented Jun 2, 2021 • edited by plaidfinch Loading

plaidfinch commented Oct 20, 2021

plaidfinch commented Oct 20, 2021 • edited by yaymukund Loading

yaymukund commented Nov 2, 2021

plaidfinch left a comment •

edited

Loading

sdleffler commented Jun 2, 2021 •

edited by plaidfinch

Loading

plaidfinch commented Oct 20, 2021 •

edited by yaymukund

Loading