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

Add FromTypeParam #28

Closed
upsuper opened this issue Apr 26, 2018 · 6 comments · Fixed by #30
Closed

Add FromTypeParam #28

upsuper opened this issue Apr 26, 2018 · 6 comments · Fixed by #30
Assignees

Comments

@upsuper
Copy link
Contributor

upsuper commented Apr 26, 2018

TypeParam may also have attributes, so it would be good to have a trait for that as well.

@upsuper
Copy link
Contributor Author

upsuper commented Apr 26, 2018

(I can probably work on this myself this weekend if no one beats me.)

@TedDriggs
Copy link
Owner

I didn't realize attributes were allowed on type params. Do you have an example of how you'd want to use this?

@upsuper
Copy link
Contributor Author

upsuper commented Apr 27, 2018

Yes.

In Servo's code, we have a derivable trait called ToCss for serializing a value of the type into CSS form. Generally, when the derived type has generic parameters, we generate where clauses for them assuming they also have ToCss.

For example:

#[derive(ToCss)]
struct Foo<Bar> {
    x: Bar,
}

will generate something like

impl<Bar> ToCss for Foo<Bar>
where
    Bar: ToCss,
{
    fn to_css<W>(dest: &mut W) -> fmt::Result where W: fmt::Write {
        x.to_css(dest)?;
        Ok(())
    }
}

In some cases, there are fields we don't want to have in the serialization. For those cases, we use #[css(skip)] attribute to skip them.

However, if a field we want to skip is the only user of a generic parameter, it makes no sense to require that generic parameter to implement ToCss, e.g. for

#[derive(ToCss)]
struct Foo<Bar> {
    #[css(skip)]
    x: Bar,
    y: u32,
}

we would want just

impl<Bar> ToCss for Foo<Bar> {
    fn to_css<W>(dest: &mut W) -> fmt::Result where W: fmt::Write {
        y.to_css(dest)?;
        Ok(())
    }
}

since whether Bar implements ToCss doesn't matter at all.

You can always try hard and scan all types to determine whether we need to bind something, but it would be easier if we can simply specify that a generic parameter don't need to be bound, like

#[derive(ToCss)]
struct Foo<#[css(no_bound)] Bar> {
    #[css(skip)]
    x: Bar,
    y: u32,
}

That being said, it seems attributes on type parameter was just stablized in rust-lang/rust#48851, and will come with Rust 1.26. It is not usable before that.

upsuper added a commit to upsuper-forks/darling that referenced this issue Apr 30, 2018
@TedDriggs
Copy link
Owner

That being said, it seems attributes on type parameter was just stablized in rust-lang/rust#48851, and will come with Rust 1.26. It is not usable before that.

That shouldn't cause any breaking changes for darling though, right? We're not changing the version of the syn dependency or anything, so I'd expect it to work for all current users; they just can't put attributes on type parameters without updating Rust first.

@upsuper
Copy link
Contributor Author

upsuper commented May 1, 2018

That shouldn't cause any breaking changes for darling though, right? We're not changing the version of the syn dependency or anything, so I'd expect it to work for all current users; they just can't put attributes on type parameters without updating Rust first.

That's right. I realized this when I wrote the test. As far as syn is able to parse it, everything is fine. Users would need to have 1.26+ to be able to use this feature, but it shouldn't anything exists.

upsuper added a commit to upsuper-forks/darling that referenced this issue May 2, 2018
TedDriggs pushed a commit that referenced this issue May 3, 2018
* Implement FromTypeParam.

This fixes #28.
@TedDriggs
Copy link
Owner

@upsuper I tried this today on 1.26 stable and it failed saying the feature wasn't stable. I think it got pushed back to 1.27, so I'm going to ship as-is but not try to ship darling's own new usage of the feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants