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

Fix interface generation and abstract classes #331

Closed

Conversation

@thinkbeforecoding
Copy link
Contributor

thinkbeforecoding commented Jan 30, 2020

This is a fix for interfaces generation for Generative provider. #211

The code instanciated the ILMethodBuilder before checking whether the method is abstract, which does produce an non empty VAR for the method.
The TypeAttributes where not automatically correctly set for interfaces.
MethodAttributes where not automatically set for interface or abstract classes.

panesofglass and others added 15 commits Mar 15, 2018
@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Jan 30, 2020

At first, I fix the method attributes just a IL generation... but it caused a problem at design time (the compiler was complaining that the method was sealed !).
So I moved the attribute fix inside the ProvidedMethod, and it works.

I also avoid to set Sealed on the type when it's an Interface.

@sergey-tihon

This comment has been minimized.

Copy link
Member

sergey-tihon commented Jan 30, 2020

@thinkbeforecoding can we merge #213 here and ensure that test works now?

@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Jan 30, 2020

Ah yes, that would be neat

@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Jan 30, 2020

I updated the PR with tests from #213

@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Jan 30, 2020

There seem to be some issues with abstract classes. But it shouldn't be difficult to fix

@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Jan 31, 2020

Yes ! It's passing now. I simply force abstract attribute on methods that have no method impl.

@sergey-tihon sergey-tihon changed the title Fix interface generation Fix interface generation and abstract classes Jan 31, 2020
@panesofglass

This comment has been minimized.

Copy link
Contributor

panesofglass commented Jan 31, 2020

@thinkbeforecoding, thank you for tackling this! Great work!

@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Feb 3, 2020

Is there anything more to do to get it merged ? 😄

@sergey-tihon sergey-tihon requested a review from dsyme Feb 3, 2020
@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Feb 10, 2020

The creation of the interface was working, but there was still some problems with with implementing it on a class with DefineMethodOverride..
Especially:

  • at the moment of the call of DefineMethodOverride, several ILTypeRef/ILTypeSpec instance where generated for the same method return type, but did not implement GetHashCode and Equals correctly. The generation failed because a method that should exist did not exist in the dictionary
  • call of protected members in method body failed because reflection was looking only for public members
@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Feb 10, 2020

With this version I have a working from scratch WsdlProvider sdk.
It parses the Wsdl using System.Xml.Linq and produces all data types, the interface with System.ServiceModel attributes, and the client class that derives from ClientBase<'t> where 't is the service interface, same way the original WsdlTp does, but here without using svcutil and the C# compiler. It is faster and less hacky.

@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Feb 11, 2020

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Feb 16, 2020

Looks like conflicts need to be resolved here?

@thinkbeforecoding

This comment has been minimized.

Copy link
Contributor Author

thinkbeforecoding commented Feb 16, 2020

This is because #332 already contains all these changes. It should be ok to just close this one

@dsyme dsyme closed this Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.