Skip to content
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

The implementation From<&[T; N]> for Option<&[T]> seems missing #125600

Closed
minghuaw opened this issue May 27, 2024 · 7 comments
Closed

The implementation From<&[T; N]> for Option<&[T]> seems missing #125600

minghuaw opened this issue May 27, 2024 · 7 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@minghuaw
Copy link

minghuaw commented May 27, 2024

The following code currently does not compile

fn foo<'a>(a: impl Into<Option<&'a [i32]>>) { }

fn main() {
    // error[E0277]: the trait bound `Option<&[i32]>: From<&[{integer}; 3]>` is not satisfied
    foo(&[1,2,3]);

    // error[E0277]: the trait bound `Option<&[i32]>: From<Option<&[{integer}; 3]>>` is not satisfied
    foo(Some(&[1,2,3]));
}

Kindly raising this as an issue because something similar was recently merged #113489

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 27, 2024
@minghuaw minghuaw changed the title The implementation From<&[T; N]> for Option<&[T]> is missing The implementation From<&[T; N]> for Option<&[T]> seems missing May 27, 2024
@demurgos
Copy link

demurgos commented May 27, 2024

  1. From a high-level point of view, I think that it's actually consistent with Rust's approach to API design to discourage such API overloading. I think that foo's signature should accept a plain option. This also the feedback you received on IRLO.

  2. More specifically regarding your request, I don't think this is analogous to impl From<&[T; N]> for Cow<[T]> #113489. The role of Cow is to manage optional borrowing. Cow behaves somewhat like &T, since [T; N] converts to &[T] it was a bit inconsistent for it to not convert into Cow<[T]>. What you are proposing here is actually a specific instance of the more general case of converting from T to Option<B> where T: Borrow<B> or T: Deref<Target=B>. It is very surprising (at least to me) for such a conversion to do perform both borrowing and Some-wrapping.

@minghuaw
Copy link
Author

Regarding consistency, I would argue otherwise with the example below. Isn't it inconsistent that &[1,2,3] is sometimes treated as &[i32] (ie. bar in the example below) and sometimes treated as &[i32; 3] (ie. foo in the example below)?

fn bar<'a>(a: Option<&'a [i32]>>) { }

fn foo<'a>(a: impl Into<Option<&'a [i32]>>) { }

fn main() {
    bar(Some(&[1, 2, 3])); // This works fine
    foo(Some(&[1, 2, 3])); // This would complain
    foo(&[1, 2, 3]); // This would complain as well
}

Isn't it more intuitive that something that can be accepted as Option<&[T]> should also impl Into<Option<&[T]>>.

@jieyouxu jieyouxu added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-discussion Category: Discussion or questions that doesn't represent real issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 29, 2024
@demurgos
Copy link

demurgos commented Jun 3, 2024

I see where you're coming from and why it's surprising. I still feel that this is trying to do too much in a single step, but I may also biased against callee-side conversions and prefer to perform them on the caller side. In particular, I avoid relying on impl<T> From<T> for Option<T>.

Given your last example, it may be acceptable for consistency.

@minghuaw
Copy link
Author

minghuaw commented Jun 3, 2024

This is probably not needed in your use cases, but it doesn't mean other people won't need it. Conversion from &[T; N] into &[T] is the only inconsistency I have seen in my use cases so far, so i don't think it's doing too much in a single step.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2024

Your issue is more fundamental than a few "missing" trait impls. Unfortunately the compiler cannot figure out that there is an impl that could apply, because it doesn't try to coerce after choosing a type for a generic parameter. Your change would allow this one special case, while still not allowing all other kinds of coercion like function item to pointer, closure to function pointer, type to dyn trait, ...

I don't know if this issue is solveable in the trait solver, or if it would require allowing people to write trait bounds using the Unsize traits. I think we should not add individual impls for certain cases without considering the entire problem and inconsistencies it will leave around.

This may also be a breaking change. I can't come up with a test right now, but it seems like it would cause existing code to fail with type annotations required where it worked fine before

@minghuaw
Copy link
Author

minghuaw commented Jun 4, 2024

@oli-obk I agree that this is more than just missing some trait impls, which is also why I had been fine with using &[1, 2, 3][..] before. It was really when I saw the #113489 that makes me think something similar could be done for Option<&[T]>. Given my limited knowledge about the rust compiler, I would love to get more insights about the difference for these two.

I tried to find rationale behind the current design choice within the community but failed to get much help. Folks often simply end up suggesting not using impl Into<Option<&[T]>> entirely (like my conversion with @demurgos above). Thus I figured opening an issue and PR would probably get me some more insights from the team.

This may also be a breaking change. I can't come up with a test right now, but it seems like it would cause existing code to fail with type annotations required where it worked fine before

That was my original concern as well, which was why I opened that PR, and I was surprised the PR passed all tests

@minghuaw
Copy link
Author

minghuaw commented Jun 4, 2024

Please correct me if I'm wrong. My understanding, with my limited knowledge on the compiler, is that the current limitation is caused by the compiler failing to coerce &[T; N] into &[T] when the argument is impl Into<Option<&[T]>>, and adding a trait impl is simply putting a bandaid on the problem, and this band-aid would likely introduce breaking change (that is not covered by current tests) and/or make future changes to the trait solver.

And this is different from the Cow<'a, [T]> case because Cow doesn't impl Cow<'a, T>: From<T>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants