core/either: Remove `EitherName` in favor of `Either` #2798

thomaseizinger · 2022-08-04T12:04:27Z

Description

Links to any relevant issues

#2650

Open Questions

Change checklist

I have performed a self-review of my own code
~~- [ ] I have made corresponding changes to the documentation~~
~~- [ ] I have added tests that prove my fix is effective or that my feature works~~
A changelog entry has been made in the appropriate crates

core/CHANGELOG.md

mxinden · 2022-08-08T06:10:10Z

This does not compile because the blanket impl for ProtocolName over anything that implements AsRef<u8> is conflicting with the implementation on Either.

I don't have a strong opinion on this. Given the complication above I am inclined to just leave it as is. Any additional benefits of this patch beyond consistency?

thomaseizinger · 2022-08-08T08:19:54Z

This does not compile because the blanket impl for ProtocolName over anything that implements AsRef<u8> is conflicting with the implementation on Either.

I don't have a strong opinion on this. Given the complication above I am inclined to just leave it as is. Any additional benefits of this patch beyond consistency?

I'll give it a bit more thought but I don't think so.

Instead of just adding impls that we need, I can see to refactor some code to streamline, how we generate ProtocolNames.

It is a bit weird that we allow so many things to be ProtocolNames and on the trait, we define two requirements that aren't really enforced:

Max length of 140
Must start with /

Maybe we should introduce a ProtocolName newtype?

mxinden · 2022-08-09T07:40:11Z

On the meta level, I think we could do better modelling protocol names.

Part of the problem is the conflict between multistream select and libp2p-identify on the specification level. One accepts a [u8], the other expects a String.

Maybe we should introduce a ProtocolName newtype?

Or a unit type for each protocol name, each implementing ProtocolName.

thomaseizinger · 2022-08-09T07:52:40Z

On the meta level, I think we could do better modelling protocol names.

Part of the problem is the conflict between multistream select and libp2p-identify on the specification level. One accepts a [u8], the other expects a String.

Damn. Of course it wasn't that easy to fix :)

Maybe we should introduce a ProtocolName newtype?

Or a unit type for each protocol name, each implementing ProtocolName.

Nice idea but it won't allow us to enforce invariants, right?

mxinden · 2022-08-10T06:26:47Z

Nice idea but it won't allow us to enforce invariants, right?

True. Would be great to enforce these at runtime. Would be wonderful to enforce these at compile time 😇

thomaseizinger · 2022-08-10T06:45:07Z

Nice idea but it won't allow us to enforce invariants, right?

True. Would be great to enforce these at runtime. Would be wonderful to enforce these at compile time 😇

Something for a future issue? I have some ideas :)

To progress on this issue, would it be okay to transition all/most usages to std types like &static [u8] and String?

mxinden · 2022-08-13T01:52:52Z

Nice idea but it won't allow us to enforce invariants, right?

True. Would be great to enforce these at runtime. Would be wonderful to enforce these at compile time innocent

Something for a future issue? I have some ideas :)

👍

To progress on this issue, would it be okay to transition all/most usages to std types like &static [u8] and String?

Given that Either also implements AsRef<[u8]> when the two subtypes implement AsRef<[u8]>, is the implementation of ProtocolName on Either still needed?

https://docs.rs/either/latest/either/enum.Either.html#impl-AsRef%3C%5BTarget%5D%3E

thomaseizinger · 2022-08-16T06:57:36Z

To progress on this issue, would it be okay to transition all/most usages to std types like &static [u8] and String?

Given that Either also implements AsRef<[u8]> when the two subtypes implement AsRef<[u8]>, is the implementation of ProtocolName on Either still needed?

https://docs.rs/either/latest/either/enum.Either.html#impl-AsRef%3C%5BTarget%5D%3E

(Un)fortunately yes. I remember trying this and it is not working.

I think the reason is in Rust's explicit strong-typing system. Just because something implements all super traits of a trait A does not mean it implements trait A by itself unless it is actively declared as such. In Golang, this would work :)

mxinden · 2022-08-19T04:49:12Z

To progress on this issue, would it be okay to transition all/most usages to std types like &static [u8] and String?

I am fine with that.

thomaseizinger · 2022-08-19T09:28:33Z

core/CHANGELOG.md

@@ -6,13 +6,16 @@
 - Change `StreamMuxer` interface to be entirely poll-based. All functions on `StreamMuxer` now
  require a `Context` and return `Poll`. This gives callers fine-grained control over what they
  would like to make progress on. See [PR 2724] and [PR 2797].
+- Remove `EitherName` in favor of `Either` from the `either` crate. See [PR 2798].
+- Replace blanket `ProtocolName` impl with specific ones on `&'static [u8]` and `Cow<'static, [u8]>`. See [PR 2798].


We could split this out into a different PR if you want.

I am fine with a single pull request only.

…dler

mxinden

I would actually suggest making String the default protocol representation. See #2831. What do you think @thomaseizinger?

mxinden · 2022-08-20T04:58:37Z

core/CHANGELOG.md

@@ -6,13 +6,16 @@
 - Change `StreamMuxer` interface to be entirely poll-based. All functions on `StreamMuxer` now
  require a `Context` and return `Poll`. This gives callers fine-grained control over what they
  would like to make progress on. See [PR 2724] and [PR 2797].
+- Remove `EitherName` in favor of `Either` from the `either` crate. See [PR 2798].
+- Replace blanket `ProtocolName` impl with specific ones on `&'static [u8]` and `Cow<'static, [u8]>`. See [PR 2798].


I am fine with a single pull request only.

mxinden · 2022-08-20T04:59:16Z

core/src/upgrade.rs

+    B: ProtocolName,
+{
+    fn protocol_name(&self) -> &[u8] {
+        ::either::for_both!(self, inner => inner.protocol_name())


mxinden · 2022-08-20T05:00:34Z

core/CHANGELOG.md

@@ -6,13 +6,16 @@
 - Change `StreamMuxer` interface to be entirely poll-based. All functions on `StreamMuxer` now
  require a `Context` and return `Poll`. This gives callers fine-grained control over what they
  would like to make progress on. See [PR 2724] and [PR 2797].
+- Remove `EitherName` in favor of `Either` from the `either` crate. See [PR 2798].
+- Replace blanket `ProtocolName` impl with specific ones on `&'static [u8]` and `Cow<'static, [u8]>`. See [PR 2798].


Why not implement ProtocolName for String and str?

thomaseizinger · 2022-08-20T07:02:53Z

I would actually suggest making String the default protocol representation. See #2831. What do you think @thomaseizinger?

Yeah we can move to string! Should we do that as part of this PR or make another one? It is getting a bit OT from the original goal haha.

I am happy to fix #2831 first and then come back to this!

mxinden · 2022-08-23T06:26:39Z

I think the following order makes most sense:

misc/multistream-select: Treat protocol as String and not [u8] #2831
Move ProtocolName to String
Do the actual change here, namely to use Either

That said, I don't think we have to stick with that order.

thomaseizinger · 2022-08-23T06:31:26Z

I think the following order makes most sense:
1. [misc/multistream-select: Treat protocol as `String` and not `[u8]` #2831](https://github.com/libp2p/rust-libp2p/issues/2831)

2. Move `ProtocolName` to `String`

3. Do the actual change here, namely to use `Either`
That said, I don't think we have to stick with that order.

I agree, leave it with me 😊

thomaseizinger · 2022-11-02T03:52:44Z

Closing in favor of #2966.

core/either: Remove EitherName in favor of Either

74f3e10

thomaseizinger requested a review from mxinden August 4, 2022 12:04

thomaseizinger commented Aug 4, 2022

View reviewed changes

core/CHANGELOG.md Outdated Show resolved Hide resolved

thomaseizinger added 2 commits August 4, 2022 22:04

Update core/CHANGELOG.md

28bbbf4

Format code

1d86e3c

thomaseizinger added 4 commits August 19, 2022 11:20

Transition UpgradeInfo::Info to &'static [u8] where feasible

21205fd

Replace blanket ProtocolName impl with specific ones

8016a0e

Add changelog entry for ProtocolName change

31956ac

Merge branch 'master' into replace-either-name-with-either

8db6355

thomaseizinger commented Aug 19, 2022

View reviewed changes

thomaseizinger marked this pull request as ready for review August 19, 2022 09:29

thomaseizinger requested review from mxinden and removed request for mxinden August 19, 2022 09:29

thomaseizinger added 3 commits August 19, 2022 11:34

Transition from String to &'static [u8]' for PendingConnectionHan…

7d646d7

…dler

Don't use &str for ProtocolName in docs

7e76ab1

Appease clippy

e116f05

mxinden mentioned this pull request Aug 20, 2022

misc/multistream-select: Treat protocol as String and not [u8] #2831

Closed

mxinden reviewed Aug 20, 2022

View reviewed changes

thomaseizinger closed this Nov 2, 2022

thomaseizinger deleted the replace-either-name-with-either branch November 17, 2022 01:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/either: Remove `EitherName` in favor of `Either` #2798

core/either: Remove `EitherName` in favor of `Either` #2798

thomaseizinger commented Aug 4, 2022 •

edited

Loading

mxinden commented Aug 8, 2022

thomaseizinger commented Aug 8, 2022

mxinden commented Aug 9, 2022

thomaseizinger commented Aug 9, 2022

mxinden commented Aug 10, 2022

thomaseizinger commented Aug 10, 2022

mxinden commented Aug 13, 2022

thomaseizinger commented Aug 16, 2022 •

edited

Loading

mxinden commented Aug 19, 2022

thomaseizinger Aug 19, 2022

mxinden Aug 20, 2022

mxinden left a comment

mxinden Aug 20, 2022

mxinden Aug 20, 2022

mxinden Aug 20, 2022

thomaseizinger commented Aug 20, 2022

mxinden commented Aug 23, 2022

thomaseizinger commented Aug 23, 2022

thomaseizinger commented Nov 2, 2022

core/either: Remove EitherName in favor of Either #2798

core/either: Remove EitherName in favor of Either #2798

Conversation

thomaseizinger commented Aug 4, 2022 • edited Loading

Description

Links to any relevant issues

Open Questions

Change checklist

mxinden commented Aug 8, 2022

thomaseizinger commented Aug 8, 2022

mxinden commented Aug 9, 2022

thomaseizinger commented Aug 9, 2022

mxinden commented Aug 10, 2022

thomaseizinger commented Aug 10, 2022

mxinden commented Aug 13, 2022

thomaseizinger commented Aug 16, 2022 • edited Loading

mxinden commented Aug 19, 2022

thomaseizinger Aug 19, 2022

Choose a reason for hiding this comment

mxinden Aug 20, 2022

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mxinden Aug 20, 2022

Choose a reason for hiding this comment

mxinden Aug 20, 2022

Choose a reason for hiding this comment

mxinden Aug 20, 2022

Choose a reason for hiding this comment

thomaseizinger commented Aug 20, 2022

mxinden commented Aug 23, 2022

thomaseizinger commented Aug 23, 2022

thomaseizinger commented Nov 2, 2022

core/either: Remove `EitherName` in favor of `Either` #2798

core/either: Remove `EitherName` in favor of `Either` #2798

thomaseizinger commented Aug 4, 2022 •

edited

Loading

thomaseizinger commented Aug 16, 2022 •

edited

Loading