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

Generic types #425

Closed
wants to merge 1 commit into from
Closed

Generic types #425

wants to merge 1 commit into from

Conversation

turion
Copy link
Contributor

@turion turion commented Jan 19, 2022

Fixes #424 . I'm trying to implement support for generic/polymorphic types. Can you give me some feedback whether I'm on the right track? If yes, I'll proceed with the other macros.

@evnu evnu requested a review from a team January 19, 2022 11:06
@evnu
Copy link
Member

evnu commented Jan 24, 2022

Can you give me some feedback whether I'm on the right track?

This looks like the right approach to me. Is something missing? How does it interact with lifetimes?

@turion
Copy link
Contributor Author

turion commented Jan 24, 2022

Is something missing?

I don't know whether this feature is now already sufficiently tested, or whether I should write an extra test somewhere, and if yes what it should contain.

How does it interact with lifetimes?

So far only on the syntactical level. I didn't implement lifetime polymorphism here because I don't understand yet how they interact with the lifetimes of the Encoder and Decoder traits. The only reason I touched them a bit is because syntactically they are declared in the same place as type parameters.

@turion
Copy link
Contributor Author

turion commented Jan 24, 2022

Is something missing?

Other derive macros are missing, e.g. for structs and enums.

@turion turion force-pushed the dev_generics branch 6 times, most recently from 3da1e00 to 599cf04 Compare January 27, 2022 14:35
@turion
Copy link
Contributor Author

turion commented Jan 27, 2022

Ok, this should be good to go now. I added type parameters to all cases where it made sense (unit enums didn't make sense), and added appropriate tests I think.

@turion
Copy link
Contributor Author

turion commented Feb 7, 2022

Ping

@evnu evnu self-requested a review February 7, 2022 15:51
Copy link
Member

@evnu evnu left a comment

Choose a reason for hiding this comment

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

I like where this is going. If we could also lift the restriction on a single lifetime, that would be great.

rustler_codegen/src/context.rs Show resolved Hide resolved
) -> GenericUntaggedEnum<GenericMap<i32, i32>, &str> {
generic_untagged_enum
}

Copy link
Member

Choose a reason for hiding this comment

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

A test with explicit lifetimes would be nice. Something along these lines maybe? Testing with lifetimes could be tricky, but a 'static string might suffice.

#[derive(NifStruct)]
struct Slice<'a> {
  my_str: &'a str
}

#[rustler::nif]
pub fn from_a_string_slice() -> Slice {
  Slice { my_str: "hello world" }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean an explicit lifetime together with a type parameter? I (hope I) haven't changed anything about the behaviour with lifetimes here. I'll send a short PR ahead that adds a lifetime test (since it seems there isn't one yet in test_codegen.rs) and rebase onto that, then we know that I didn't break them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, seems like lifetimes are broken anyways, #428. This looks like an orthogonal issue to me.

@turion turion mentioned this pull request Feb 10, 2022
@turion
Copy link
Contributor Author

turion commented Mar 17, 2022

Ping @evnu ;)

@evnu
Copy link
Member

evnu commented Mar 17, 2022

Ping @evnu ;)

Do you plan to complete #430 first and then we take a look into this PR here again?

@turion
Copy link
Contributor Author

turion commented Mar 18, 2022

Do you plan to complete #430 first

Doesn't look like it. It's a too complicated issue, and orthogonal in content to this here. (But there will be merge conflicts)

@turion
Copy link
Contributor Author

turion commented Aug 25, 2022

Clippy fails, but I didn't touch the failing code 😕 . The same with the formatter.

@evnu
Copy link
Member

evnu commented Sep 11, 2022

@turion can you rebase on master?

@turion
Copy link
Contributor Author

turion commented Sep 14, 2022

@evnu The lifetimes MR has changed so much that this PR here can basically be thrown away. I'm not even sure, maybe generic types are already handled now?

@turion turion closed this Sep 14, 2022
@evnu
Copy link
Member

evnu commented Sep 15, 2022

I'm not even sure, maybe generic types are already handled now?

We will have to try how well
adds generic types, but maybe we need to test this further. Lets keep this PR closed. For #424, we will have to check if that is actually fixed by #483.

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.

NifTuple doesn't work with polymorphic structs
2 participants