Skip to content
This repository was archived by the owner on Apr 15, 2022. It is now read-only.

TypeName doesn't account for generics #12

Closed
randomPoison opened this issue Apr 2, 2020 · 2 comments · Fixed by #15
Closed

TypeName doesn't account for generics #12

randomPoison opened this issue Apr 2, 2020 · 2 comments · Fixed by #15
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@randomPoison
Copy link
Owner

TypeName is intended to uniquely identify a fully-resolved type, such that it can be used to reliably relate type references within a schema. However, a generic type will have the same type name for all concrete instantiations, violating this requirement.

The correct solution here is to take generic parameters into account in some way. A simple solution would be to include an additional list of the type TypeName values for each generic parameter. This would allow TypeName to be as deeply-nested as needed in order to fully identify the type. For example, TypeName would change to something like:

pub struct TypeName {
    pub name: Cow<'static, str>,
    pub module: Cow<'static, str>,
    pub type_params: Vec<TypeName>,
}

My only reservation with this approach is that getting the TypeName for a type would no longer be a const-compatible operation, since it requires populating a Vec. In practice, though, I don't think we need creating a TypeName value to be a const operation, and none of the places where we currently create a TypeName value would be broken by the change.

In making this change, we will probably need to add a type_name method on the Describe trait. In order to get the fully-resolved name and module for type parameters, we can't just use the type_name! macro since it can't resolve the module of external types.

@randomPoison randomPoison added enhancement New feature or request good first issue Good for newcomers labels Apr 2, 2020
@randomPoison
Copy link
Owner Author

randomPoison commented Apr 7, 2020

Thinking about it some, we could probably have the type params be a Cow<'static, [TypeName]>. I think the list of type names should be statically known, so we can possibly get away without allocating inside Describe::describe. This would definitely be the case if we added a NAME const member to the Describe trait.

@randomPoison randomPoison self-assigned this May 15, 2020
@randomPoison
Copy link
Owner Author

I've got an initial version of this implemented using a type_name associated function. I tried using the Cow<[TypeName]> approach, however it appears that rust-lang/rust#47032 is preventing structs with that contain a Cow<[Self]> field to not compile. For now we'll have to stick with Vec<TypeName> and an associated function. Once that compiler issue is fixed, we can switch to using an associated constant instead.

@randomPoison randomPoison added blocked The ticket is blocked on another ticket or an external dependency and removed blocked The ticket is blocked on another ticket or an external dependency labels May 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant