Remove AsRef/AsMut for [T; N] due to effects to all (even indirect) dependents
#132
Conversation
|
Let's talk about this on #131 first. This is a very, very big breaking change which will impact a large part of the RustCrypto codebase. If you really want to see it through, I would suggest opening PRs against all of the repos to see the impact. |
|
Of course, it has to be discussed first. I solely opened it so if we agree on the idea, this is available. I can make other PRs as well to prove lack of or minimal impact :) |
@kayabaNerve I specifically replaced all of the occurrences of I'm asking you to do this not to "prove lack of or minimal impact", but to address the actual downstream breakages I know this is going to cause. This is a feature we are actively using which you are asking us to remove. |
|
I apologize if I was too optimistic due to my experience, and if I haven't realized how impactful this would be. I've tried to be clear this isn't my place to comment on if it should occur, solely to raise the annoyance I observed. If you don't believe this is justified, please close both the issue and the PR and I'll have no issue, solely an annoyance I'll tolerate. I apologize if this has been too much of a bother. I'll make the PRs now to discover and realize the impact otherwise. |
|
The real issue is that this is a useful feature which we are using which doesn't have a good replacement which you are asking to remove. The alternative reference conversion looks a bit like That said, the usefulness of the feature does need to be balanced against the kind of problems you're encountering in #131, which is why I'm trying to figure out in that thread what exactly is going on in your code which is leading to the ambiguity in the first place. |
|
For each of the following repositories, I added the following block to their root-level I then ran This is comprehensive to every So the |
|
That's surprising. I would've expected more breakages.
|
|
I'm pleasantly surprised by it given your recent comment, yet it lines up with my initial optimistic view. My guess is because I left If this did cause a notable amount of breakage, I'd concede the issue as obviously, this would be a used feature. My entire issue is premised on how this feature may not offer sufficient benefit to justify the unfortunate annoyance it may cause. It's not to say this isn't a feature itself. I'll open the three corresponding PRs. I trust you to decide if it's better to have this feature or more pleasant to remove it. I won't hold any 'grudge' either way. I understand that while |
That may very well be it. That said, it could still be a useful tool for people who want to write code in terms of core arrays rather than the |
|
In that case, there is still I do understand why |
|
Three required PRs have been opened. |
|
I might do a slightly different take on this PR that accomplishes the same result |
| /// pub fn get_third_item_const_generic<T, const N: usize>(arr_ref: &[T; N]) -> &T | ||
| /// where | ||
| /// [T; N]: AssocArraySize + AsRef<ArrayN<T, N>> | ||
| /// { | ||
| /// get_third_item_hybrid_array(arr_ref.as_ref()) | ||
| /// } |
There was a problem hiding this comment.
This is the really sad thing to lose, BTW. It made it possible to write APIs using const generics that hid the use of Array behind the scenes, so the callers didn't have to care about Array at all
See #131, #132 Notably these impls even existing can break code which would otherwise compile when `hybrid-array` is included as a dependency, due to some complex interactions between type inference and the sized-to-unsized coercion, something I previously knew little about prior to #131: https://rustc-dev-guide.rust-lang.org/traits/unsize.html Notably the existence of overlapping `AsRef` impls can break inference in cases where code is depending on the sized-to-unsized coercion, since the compiler can infer the desired type is `[T]` and coerce the other arguments from `[T; N]` into `[T]` to match. When this is no longer possible, it can fail in some pretty counterintuitive ways. See #131 for examples. This is all unfortunate because it's removing functionality which made it possible to expose `[T; N]` in public APIs so the caller didn't have to worry about `Array` at all. Perhaps we can lean on the `From` impls for doing such conversions instead.
|
I opened #133 which ended up being more similar to this PR than I was hoping and I guess I could've just told you to do those changes @kayabaNerve, mea culpa. I'd like to see if I can add back the documentation example at least, using |
Closes #131, #132 Notably these impls even existing can break code which would otherwise compile when `hybrid-array` is included as a dependency, due to some complex interactions between type inference and the sized-to-unsized coercion, something I previously knew little about prior to #131: https://rustc-dev-guide.rust-lang.org/traits/unsize.html Notably the existence of overlapping `AsRef` impls can break inference in cases where code is depending on the sized-to-unsized coercion, since the compiler can infer the desired type is `[T]` and coerce the other arguments from `[T; N]` into `[T]` to match. When this is no longer possible, it can fail in some pretty counterintuitive ways. See #131 for examples. This is all unfortunate because it's removing functionality which made it possible to expose `[T; N]` in public APIs so the caller didn't have to worry about `Array` at all. Perhaps we can lean on the `From` impls for doing such conversions instead.
|
Equivalent outcome accomplished in #133 |
|
No worries! I'm not offended and knew when I deleted the documentation, this probably wasn't optimal. Thanks for making the changes yourself! |
Resolves #131. I did maintain the
From<&[u8]>references, which still offers a way to achieve the removed functionality (albeit not with the existing traits). I cannot comment this should be merged, solely lodge my issue and note this resolves my issue. I'll accept being told I'm the one who's misusing things and this is intended behavior 😅This doesn't break
sha2 0.11.0-rc.0,blake2 0.11.0-rc.0as used in my tree so hopefully it wouldn't actually be problematic to move forward with.