Skip to content

Conversation

@maddiemort
Copy link
Contributor

@maddiemort maddiemort commented Nov 3, 2023

Motivation

I'm working on an implementation of Subscriber that needs to be able to store (in a form as close as possible to the original) the inputs to its methods, and reconstruct them later to be passed to another Subscriber. For the most part, this works really well, but the biggest sticking point by far came from trying to reconstruct a ValueSet from a stored list of (Field, Value) pairs.

The core of the problem is the ValidLen trait and its usage by FieldSet::value_set(). In order to call that function, you have to be able to provide a statically-sized array, because that's the only type that implements ValidLen.

While, in theory, constructing an array for my use case should be just about possible (although the tricks necessary to do so likely involve a macro and the reimposition of an arbitrary length limit), lifetime issues (that appear to result from a call to <[_; N]>::try_from()) make it impossible. I'm going to construct a minimal example of this and investigate further at some point, but in the meantime:

Solution

Since #2508 removed the limitation on array length here, there doesn't seem to be a drawback to simply adding an implementation of ValidLen for slices. They already meet the Borrow supertrait requirement, so no additional machinery is needed.

This makes this API trivially easy to call for my use case, and I can't think of any associated drawbacks.

@maddiemort maddiemort requested review from a team and hawkw as code owners November 3, 2023 13:45
@maddiemort
Copy link
Contributor Author

I'm not entirely sure why CI is failing, but I don't think it's related to my change.

@hawkw
Copy link
Member

hawkw commented Nov 3, 2023

This change makes sense to me. The CI failure is due to a yanked dependency breaking our build with -Z minimal-versions, that's not related to this change.

@maddiemort
Copy link
Contributor Author

Is there anything I can do to help get this merged?

@nicholastmosher
Copy link

Chiming in to show support for this addition, having this impl upstream will allow us to drop a long-standing patch to a fork of tracing 🙏

There used to be a length restriction on the arrays usable to construct
`ValueSet`s, but this was due to Rust compiler limitations that no
longer exist. After that restriction was removed, the `ValidLen` trait
was kept around but implemented for statically-sized arrays of all
lengths.

While in theory this allows `ValueSet`s to be constructed from arrays of
arbitrary length, the size of the arrays must still be known at compile
time. Now that there is no longer a restriction on the length of arrays,
slices could in theory be used as well (since they meet the supertrait
requirement of `ValidLen`), but no implementation of the trait exists.

Adding an implementation of `ValidLen` for slices is straightforward.

Refs: tokio-rs#2508
@Ryex
Copy link

Ryex commented Nov 18, 2025

Chiming in to ask for this as well.
My use case is tracing-cloudchamber. a ffi to create tracing events and spans from c++. At this moment the API is limited in the number of fields it can take due to C processor macro usage and the need for a statically sized array

The macro limit can be worked around with constexpr and templates, but that would only let me make iterable collections of fields and values from which I could make slices.

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.

4 participants