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

Implement SourceCode for Vec<u8> #216

Merged
merged 1 commit into from
Nov 7, 2022
Merged

Implement SourceCode for Vec<u8> #216

merged 1 commit into from
Nov 7, 2022

Conversation

Phantomical
Copy link
Contributor

@Phantomical Phantomical commented Oct 30, 2022

SourceCode is implemented for &str, String, and &[u8] but it is missing an impl for Vec<u8>.

@olivia-fl
Copy link
Contributor

Thoughts on just replacing the existing impls with the following?

impl SourceCode for [u8] { ... }
impl SourceCode for str { ... }
impl<T: Deref + Send + Sync> SourceCode for T  where T::Target: SourceCode { ... }

This should cover all the existing impls, Vec<u8>, and any other smart pointer types, including ones outside of std.

@zkat
Copy link
Owner

zkat commented Nov 3, 2022

wait really?

If that won't be a breaking change, that does seem better!

@olivia-fl
Copy link
Contributor

It's technically a breaking change, because it may overlap with existing downstream impls. For example, suppose the following code was in some dependent crate:

struct Foo(String);

impl Deref for Foo {
    type Target = str;
    ...
}

impl SourceCode for Foo { ... }

That SourceCode impl is now invalid, because it conflicts with the blanket impl we're adding, breaking the trait coherence rules.

RFC 1105 states that implementing traits is only considered minor though, even though it is always technically breaking. However, the type of breaking change discussed there has to do with dispatch ambiguity rather than coherence, so I'm not sure the same reasoning applies. Fixing dispatch ambiguity issues is very straightforward, while fixing this type of coherence issue might require much deeper changes.

I'm not sure how much of an issue this is in practice. I wish there was an easy way to just build all the dependent crates against a proposed change like this, similar to how crater works for rustc changes :)

@zkat zkat added the breaking A semver-major breaking change label Nov 6, 2022
@Phantomical
Copy link
Contributor Author

Phantomical commented Nov 6, 2022

Would it be alright to merge this PR by itself and then have a second one with the breaking impl based on Deref? The changes here right now are not breaking changes and it would be nice to have the impl for Vec<u8> available.

@olivia-fl
Copy link
Contributor

Yeah, when I originally suggested the Deref option I hadn't thought through the fact that it's potentially breaking.

@zkat
Copy link
Owner

zkat commented Nov 7, 2022

Let's merge this in for now, then :)

@zkat zkat merged commit c857595 into zkat:main Nov 7, 2022
@Phantomical Phantomical deleted the vec-source branch November 7, 2022 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A semver-major breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants