Skip to content

Implement Encode for sequences of string-like types #377

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tsurdevson
Copy link
Contributor

@tsurdevson tsurdevson commented Jun 10, 2025

This is in turn used to implement Encode for slices of string-like types. Introduce a marker trait for types that encode into ssh-strings, and implement Encode for various kinds of sequences of such types. E.g. implement Encode for Vec<String>, &[&str], etc. Downstream users could implement the marker trait for their own types and get the same behavior for slices of their own types.

Closes #314.

@tarcieri
Copy link
Member

I'm not sure what you're trying to accomplish with this, and notably types like str and String cannot express the full range of an RFC4251 string which is a [u8]-like bytestring. This can make these types problematic anywhere you need to be able to round-trip a message encoding, such as key comments which were recently changed from a String in #360.

@tsurdevson
Copy link
Contributor Author

I'm not sure what you're trying to accomplish with this

This was simply meant to be something I could link to as I asked a followup question to your comment on #314. I didn't have time to finish it earlier, hence the draft status and no question posted yet. In retrospect I could just as easily have linked to a commit, so I apologize for the noise of an additional PR.

I think some marker trait which denotes that encoding a message this way is actually valid for a given type would be helpful, and may even get rid of the impl conflicts

My question would have been: I'm not quite sure I understand what you mean by this. Something like this (PR)? "Encoding a message this way is actually valid" --> a trait for types that encode into an RFC4251 string?

(Judging by your answer I'm going to guess this was not quite what you had in mind.)

@tsurdevson tsurdevson closed this Jun 10, 2025
@tarcieri
Copy link
Member

Oh sorry, I was missing the context. My bad.

Maybe call it something like Rfc4251String? Can you also try adding the blanket impl from #314?

@tarcieri tarcieri reopened this Jun 10, 2025
@tsurdevson
Copy link
Contributor Author

Maybe call it something like Rfc4251String? Can you also try adding the blanket impl from #314?

Happy to rename it. What blanket impl would that be? There's already these two:

impl<T: EncodesLikeString> Encode for [T] { ... }
impl<T: EncodesLikeString> Encode for Vec<T> { ... }

With some more boilerplate it can be expanded to cover all kinds of combinations of references, slices and owned types too, like Vec<&[u8]>, &[&&[u8]], Vec<&String> etc.

@tsurdevson tsurdevson changed the title Introduce EncodesLikeString for types that encode like SSH strings Implement Encode for sequences of string-like types Jun 14, 2025
@tarcieri
Copy link
Member

I was referring to this one: #314 (comment)

You don't appear to currently have a generic impl for Vec

@tsurdevson
Copy link
Contributor Author

Only the one bounded by T: EncodesLikeString, yes. Perhaps I'm just missing the obvious, but adding impl<T: Encode> Encode for Vec<T> would still run into the impl-collision for Vec<u8>.

@tarcieri
Copy link
Member

Yeah, that seems problematic

With this change, various types like `&[&str]`, `Vec<&String>,
&[Vec<u8>`, etc. are now `Encode` by default, and encode as an RFC4251
string of the encoded entries.

Custom types can opt into this by implementing the marker trait `Rfc4251String`.

Implementation note: This would be more general and cover more types if
we could blanket `impl<T: Encode> Encode for &T`, as this would cover
any level of references in the trait bound for the `Rfc4251String`
blanket implenentation. However, this would collide with the `Label`
trait, so instead this adds explicit impls for the immediate types that
we implement `Encode` for.
@tsurdevson tsurdevson force-pushed the ssh-encode-slice-of-stringlike branch from e32c9d5 to c242dab Compare June 16, 2025 11:19
@tsurdevson
Copy link
Contributor Author

tsurdevson commented Jun 16, 2025

I've tried to clean it up a bit and make it as general as I can without stepping on the compiler's toes. Feel free to review if you think it adds anything, or just close it if it confuses more than it helps.

@tsurdevson tsurdevson marked this pull request as ready for review June 16, 2025 11:23
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.

Expand impl Encode for Vec<String> to impl Encode for Vec<E: Encode>
2 participants