[generator] Fix generated code that caused CS0169 warnings. #625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Nice! I had gotten the warnings cleaned up inside |
|
Sounds good! I am curious about how you cleaned up the warnings inside |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
This looks great, the one thing I want to do differently is the new unit tests (which are also great!). As you've seen, having unit tests that compare expected output can be a pain when you want to make changes to the code we generate, because you have to update tons of I think the 2 tests you added could be changed to variations of: or instead of adding all the new Then this should be good to go! |
|
Draft release note Here's a candidate Xamarin.Android release note for this change. For team members and the author of this PR, feel free to suggest changes or even a whole new wording. Thanks! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hey, thank you for handling the unit test changes. Its been a busy week at work, so I haven't had a chance to get back to this PR. |
…r to internal fields (like class_ref) when the class's base is in an external assembly. - This prevents a CS0109 warnings during binding generation. - Updated expected generator output files for existing tests. - Added two new tests that verify the expected output when the base class is in an external assembly and when the base class is in the same assembly as the derived class.
|
No worries! There's an upcoming deadline for getting things into 16.7 and I wanted to make sure this made it in time. 😄 |
jpobst
pushed a commit
that referenced
this pull request
Apr 23, 2020
* Modified the generator tool so that it no longer adds a 'new' modifier to internal fields (like class_ref) when the class's base is in an external assembly. - This prevents a CS0109 warnings during binding generation. - Updated expected generator output files for existing tests. - Added two new tests that verify the expected output when the base class is in an external assembly and when the base class is in the same assembly as the derived class. (cherry picked from commit ce8dc40)
jpobst
changed the title
jonpryor
pushed a commit
that referenced
this pull request
Aug 14, 2020
`generator` is an amazingly powerful tool that has proven to be very versatile over the past decade. However, many of the remaining bugs fall into a category that is hard to fix given `generator`'s current design, which is: 1. Read in Java model 2. Make tweaks to Java model 3. Open `<TYPE>.cs` 4. Loop through Java model determining what C# to write The issue with this is that we are deciding **what** to generate on-the-fly to a write-only stream. This means that once we've written to the stream we cannot change it if future information suggests that we should. A good example is issue #461. The Java model is: #461 Given: // Java: interface AnimatorListener { onAnimationEnd (int p0); onAnimationEnd (int p0, p1); } Looping through this, we see we need to generate an `EventArgs` class for `AnimatorListener.onAnimationEnd(int p0)`, so we do: public partial class OnAnimationEndEventArgs : global::System.EventArgs { int p0; public OnAnimationEndEventArgs (int p0) { this.p0= p0; } public int P0 { get { return p0; } } } Then we get to the second method `AnimatorListener.onAnimationEnd(int p0, int p1)` and see that we need to add additional parameters to `OnAnimationEndEventArgs`. However we cannot modify the already written class, so we generate a second `OnAnimationEndEventArgs` with different parameters, which causes CS0101 compilation errors in C#. Another example is we can generate an empty `InterfaceConsts` class. This is because we write the class opening because we think we have members to place in it (`public static class InterfaceConsts {`), but as we loop through the members we thought we were going to add they all get eliminated. At that point all we can do is close the class with no members. The solution to both examples above is "simple": we need to first loop through the rest of the Java model and determine what we **actually** need to generate before we start writing anything. We can then merge the two `OnAnimationEndEventArgs` classes, or forgo writing an empty `InterfaceConsts` type. However, rather than adding this additional logic in each instance that needs it, there is enough benefit to convert the entire `generator` architecture to work by first building a C# model of what needs to be generated that can be tweaked before writing to a file, resulting in a design of: 1. Read in Java model 2. Make tweaks to Java model 3. ***Build C# model from Java model*** 4. ***Make tweaks to C# model*** 5. Open `<TYPE>.cs` 6. Loop through C# model, writing code Having a C# model to tweak should also help in a few other tricky cases we fail at today, like determining `override`s (#367, #586) and implementing `abstract` methods for `abstract` classes inheriting an `interface` (#470). Additionally, having distinct logic and source writing steps will make unit testing better. Currently, the only way to test the *logic* of if we are going to generate something is to write out the source code and compare it to "expected" source code. This means tiny fixes can have many "expected" files that need to change (#625). With separate steps, we can have a set of unit tests that test what code we are writing, and a set that tests the logic of determining what to generate. To test the above "2 `EventArgs` classes" example, we can test a fix by looking at the created C# model to ensure that only a single combined `OnAnimationEndEventArgs` class is created. We would not need to write out the generated source code and compare it to expected source code. To assist in this new design, add a new `Xamarin.SourceWriter.dll` assembly which is responsible for generating the C# source code. This eliminates a lot of hard to read and error prone code such as: writer.WriteLine ("{0}{1}{2}{3}{4} unsafe {5} {6}{7} ({8})", indent, visibility, static_arg, virt_ov, seal, ret, is_explicit ? GetDeclaringTypeOfExplicitInterfaceMethod (method.OverriddenInterfaceMethod) + '.' : string.Empty, method.AdjustedName, method.GetSignature (opt)); and replaces it with: var m = new MethodWriter { IsPublic = true, IsUnsafe = true, IsOverride = true, Name = method.AdjustedName }; m.Write (…); which will emit: public unsafe override void DoSomething () { } `Xamarin.SourceWriter.CodeWriter` is a wrapper around a `System.IO.Stream` that tracks the current indent level, rather than needing to pass `indent` around to every method. cw.WriteLine ("{"); cw.Indent (); // ... write block body ... cw.Unindent (); cw.WriteLine ("}"); ~~ Testing ~~ Many existing unit tests call directly into methods like `CodeGenerator.WriteProperties()`. These methods are no longer available when using `JavaInterop`/`XAJavaInterop`. These tests were either changed to use `CodeGenerator.WriteType()` or were moved to only run for `XamarinAndroid` codegen target. Tests were updated to ignore whitespace and line endings differences, as the refactor did not preserve 100% identical whitespace.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
When generating C# bindings for Java binaries, the generator tool adds the
internal static IntPtr class_reffield to the C# class. When adding this field, the generator tool needs to determine if thenewmodifier needs to be applied to the field, which indicates that the field is purposefully shadowing theclass_reffield of the base class.The current approach to determine if the
newmodifier is needed is to check if the target Java class is eitherjava.lang.Objectorjava.lang.Throwable. If the target class is one of those two, then thenewmodifier is not applied. Because theclass_reffield is internal, this is not a thorough enough check. If the target class's base class is from a different Java binary (and thus a different api.xml), thenewmodifier is not necessary for the target class'sclass_reffield. This is because theclass_reffield is marked internal and is always hidden when it's in a different assembly. In this case, the C# class will compile with the following warning:warning CS0109: The member 'TargetClass.class_ref' does not hide an accessible member. The new keyword is not required.java.lang.Objectas the base class from an external assembly, the issue and fix still apply to other base classes from other external assemblies.Example
First, some very very simple Kotlin classes. (This is also reproducible with Java).
I then compile these classes into an Android library and build the AAR file. That AAR file is then added to a Xamarin Android Class Library project. No edits are made to any of the XML files in the Transforms folder. When the Xamarin project is built, it produces the following output:
Note how the MyBase and MyImplementor classes, which inherit from Java.Lang.Object — an external base class — produce a CS0109 warning, while MyDerived does not.
This Pull Request
This pull request implements a more thorough check when determining if the
newmodifier is necessary, updates existing unit tests to account for the new behavior, and adds new unit tests for the new behavior.About Generator Tool Changes
First, for context, the
ClassGen.InheritsObjectproperty is set by the following code in theClassGenconstructor.Previous Code
The previous check for determining if the
newmodifier is needed wasThe loop inside the if statement is not necessary. Whenever
requireNewisfalse,@classis eitherJava.Lang.ObjectorJava.Lang.Throwable, and thus doesn't have a baseclass. Therefore, the loop will never be run asbgshould always benullwhenrequireNew == falsePR Code
The previous code is replaced by the following:
This code uses the
FromXmlproperty to determine if the base class is from the same assembly. The null checks are necessary not just for handling@class == Java.Lang.Objectbut also for unit tests where aClassGenobject may not have a base class set.This code is much more succinct and clear about what it is being checked. An explanatory comment is provided in the code as well.
About Unit Test Changes
Existing Unit Test Changes
Some of the existing unit tests needed to have their expected output text files modified. This was mostly due to
ClassGenobjects in the tests not setting a base class, but still expecting thenewmodifier on theclass_reffield.New Unit Tests
The unit tests added by this PR are
WriteClassExternalBaseandWriteClassInternalBase. Both create and register theJava.Lang.Objectclass with theSymbolTableso that aClassGenobject can be created for it. Then both tests create a newClassGenobject called@classthat represents thejava.code.MyClassand validate it, which populates its base class members using theSymbolTable.The
WriteClassInternalBasetest then sets theFromXmlproperty of@class.BaseGento indicate that the base class ofjava.code.MyClasscomes from the same assembly. This is not done in theWriteClassExternalBase.The tests conclude by generating the C# code for
java.code.MyClassand comparing it to the contents of the corresponding expected results text file. The content of these text files was created by copying the content of the corresponding WriteClass.txt and modifying it as little as necessary.Conclusion
I've tested this fix for the CS0109 warning not just on the binaries from the arbitrary Kotlin example above, but also on our organization's binding projects. Additionally, all generator tool unit tests are passing. Unfortunately I was unable to setup a build environment on Windows that allows me to run all the tests contained in the solution, however, since all changes were contained to the generator tool, I believe they'll still pass.
Thank you for reviewing this pull request. Please let me know if I'm making any over-simplifications or am missing some context that would render my changes invalid. I've only had my head in this project for about a day, so I definitely don't have deep knowledge of the project and its various 'gotchas'.