dialectic icon indicating copy to clipboard operation
dialectic copied to clipboard

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

Open sdleffler opened this issue 4 years ago • 4 comments
trafficstars

sdleffler avatar Apr 08 '21 17:04 sdleffler

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.

sdleffler avatar Jun 02 '21 18:06 sdleffler

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

plaidfinch avatar Oct 20 '21 15:10 plaidfinch

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

  • [x] 1. Swap the order of the type parameters in Choose and Offer types, and fix all type errors that result. (Done in #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.

plaidfinch avatar Oct 20 '21 15:10 plaidfinch

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.

yaymukund avatar Nov 02 '21 00:11 yaymukund