Skip to content

Conversation

mendess
Copy link
Contributor

@mendess mendess commented Dec 27, 2023

Add support of traits that have generic parameters. Resolving #16.

(Also I tried to sign the Google CLA as described in the CONTRIBUTING.md file, but I kept getting a A server error occurred, please try your request again. error)

@andrewtoth
Copy link

Would it also be possible to change the generic parameter name T for the blanket implementation to something less likely to cause a naming conflict? T is commonly used as a name for a generic type, but when using this macro it will not compile unless the parameter in the trait is renamed. Similarly, for generic types used at the function level.

For example, even with this PR merged, the following does not compile:

#[trait_variant::make(Trait: Send)]
trait LocalTrait<T> {
    async fn trait_fn() -> T;
}

and

#[trait_variant::make(Trait: Send)]
trait LocalTrait {
    async fn trait_fn<T>() -> T;
}

Perhaps change it to TraitVariantBlanketType or something?

@mendess
Copy link
Contributor Author

mendess commented Dec 29, 2023

@andrewtoth that's a good idea, done.

@andrewtoth
Copy link

Thanks! I see PR #20 will have a conflict because of this now though :(.

@mendess
Copy link
Contributor Author

mendess commented Dec 29, 2023

Thanks! I see PR #20 will have a conflict because of this now though :(.

That's okay. I'll fix the conflicts! I've already done so I could test both PRs in a real codebase

Copy link
Member

@tmandry tmandry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks reasonable. Shame that I missed the trait's where clauses in the first place, but we can consider that a bug fix.

Thanks for the PR!

@tmandry tmandry merged commit 8a98a23 into rust-lang:main Jan 4, 2024
@tmandry tmandry mentioned this pull request Jan 4, 2024
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.

3 participants