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

Forward @:nativeTypeConstraints in closures to class filter #7863

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented Feb 25, 2019

I have some issues with native cs libs using type parameter constraints: while I can generally use them with @:nativeTypeConstraints (see #3526), the closures to class filter generates classes without these type constraints, resulting in a successful haxe compilation but a compilation error in C#.

This PR does two things:

  • forward @:nativeTypeConstraints meta to these classes
  • add type parameter constraints to the needed type parameters

Do not hesitate to tell me if my code doesn't make sense or if there are better ways to do it; I'm still not familiar with ocaml / haxe sources.

Related question: should @:nativeTypeConstraints become a proper meta? (I could add it to this PR)

@Simn Simn requested a review from nadako February 25, 2019 11:23
@nadako
Copy link
Member

nadako commented Feb 25, 2019

I am not really familiar (anymore) with this code, and I can't say I fully understand what's going on here, but forwarding constraints makes sense to me and since the tests still pass, it should be fine ;-)

Could you maybe add a test for this somehow?

@kLabz
Copy link
Contributor Author

kLabz commented Feb 26, 2019

Could you maybe add a test for this somehow?

Sure, I'll look into this :)

@kLabz kLabz force-pushed the fix/native-type-constraints-in-generated-closures branch from 8548000 to 0476073 Compare February 26, 2019 10:48
@kLabz
Copy link
Contributor Author

kLabz commented Feb 26, 2019

I actually dropped the @:nativeTypeConstraints custom meta and used @:nativeGen directly instead. Issues impacted: #3526 #4002 and potentially hxcs#37.

I'll add some tests, trying to reflect what was happening in my application.

@kLabz
Copy link
Contributor Author

kLabz commented Feb 26, 2019

Tests have been added and CI passed.

The code related to metadata has been rewritten, you might want to review again.

@kLabz
Copy link
Contributor Author

kLabz commented Mar 22, 2019

@Simn could this be considered for 4.0?

We would like to switch to official haxe 4.0 when it is available and possibly stick with it for a while for production apps, but our backend cannot work without this fix (or something similar).

I can work on this again as much as needed.

Edit: just rebased haxe 4 rc2 to get an updated build from appveyor (hopefully) before monday.

@kLabz kLabz force-pushed the fix/native-type-constraints-in-generated-closures branch from fc2c2d8 to 65c521a Compare March 22, 2019 15:59
@nadako nadako merged commit ade3bf6 into HaxeFoundation:development May 7, 2019
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.

2 participants