Skip to content

[generator] Fix NRE by cloning method when copying from base type to derived type. #1080

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 15, 2023
Merged

[generator] Fix NRE by cloning method when copying from base type to derived type. #1080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions tests/generator-Tests/Unit-Tests/CodeGeneratorTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ protected List<GenBase> ParseApiDefinition (string xml)
foreach (var gen in gens)
options.SymbolTable.AddType (gen);

foreach (var gen in gens)
gen.FixupAccessModifiers (options);

foreach (var gen in gens)
gen.Validate (options, new GenericParameterDefinitionList (), generator.Context);

Expand Down
49 changes: 49 additions & 0 deletions tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,55 @@ public void DontWarnIfNestedTypeNameMatchesNamespace ()
Assert.False (sb.ToString ().Contains ("warning BG8403"));
}


[Test]
public void AvoidNREOnInvalidBaseMethod ()
{
// We copy methods from the package-private base class to the public class, however
// the copied method is not valid because it doesn't understand the generic argument
// type. The method is remove from the public class, but we need to ensure the
// base class method still exists and IsValid.
var xml = @"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
</package>

<package name='android.view' jni-name='android/view'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' final='false' name='View' static='false' visibility='public' jni-signature='Landroid/view/View;' />
</package>

<package name='com.google.android.material.behavior' jni-name='com/google/android/material/behavior'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='ViewOffsetBehavior' static='false' visibility='' jni-signature='Lcom/google/android/material/appbar/ViewOffsetBehavior;'>
<typeParameters>
<typeParameter name='V' classBound='android.view.View' jni-classBound='Landroid/view/View;'>
<genericConstraints>
<genericConstraint type='android.view.View' />
</genericConstraints>
</typeParameter>
</typeParameters>
<method abstract='false' deprecated='not deprecated' final='false' name='layoutChild' jni-signature='(Landroid/view/View;)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='protected'>
<parameter name='child' type='V' jni-type='TV;' not-null='true' />
</method>
</class>

<class abstract='false' deprecated='not deprecated' extends='com.google.android.material.behavior.ViewOffsetBehavior' extends-generic-aware='com.google.android.material.behavior.ViewOffsetBehavior&lt;android.view.View&gt;' jni-extends='Lcom/google/android/material/appbar/ViewOffsetBehavior;' final='false' name='Behavior' static='false' visibility='public' jni-signature='Lcom/google/android/material/appbar/Behavior;'>
</class>
</package>
</api>";

var gens = ParseApiDefinition (xml);

var public_class = gens.Single (g => g.Name == "Behavior");
var base_class = gens.Single (g => g.Name == "ViewOffsetBehavior");

// Method got removed
Assert.AreEqual (0, public_class.Methods.Count);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this asserting that the method got removed?

OK, this test is asserting that we no longer NRE, that the BG8800 warning is still emitted (implicitly), that public_class has no methods, and base_class.Methods is still sane.

But… shouldn't the "final" fix be a situation in which:

  1. public_class.Methods.Count is also 1, and
  2. public_class.Methods[0].IsValid is true, and
  3. public_class.Methods[0].Parameters[0].JniType is java/lang/Object?

Upon review, it feels like this PR fixes the NRE, but doesn't resolve the underlying scenario allowing publicly accessible members from package-private types to be bound in the derived type.

Is that possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, this PR only fixes the NRE. It does not try to fix the underlying issue of correctly supporting generics when copying methods from a package-private class. I do not believe that is a topic we want to tackle any time soon.


// Method still exists and is valid
Assert.AreEqual (1, base_class.Methods.Count);
Assert.AreEqual (true, base_class.Methods [0].IsValid);
}

static string StripRegisterAttributes (string str)
{
// It is hard to test if the [Obsolete] is on the setter/etc due to the [Register], so remove all [Register]s
Expand Down
4 changes: 2 additions & 2 deletions tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.PublicFinalClass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public unsafe void PublicMethod ()
}
}

// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassB']/method[@name='packageMethodB' and count(parameter)=0]"
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodB' and count(parameter)=0]"
[global::Java.Interop.JniMethodSignature ("packageMethodB", "()V")]
public unsafe void PackageMethodB ()
{
Expand All @@ -51,7 +51,7 @@ public unsafe void PackageMethodB ()
}
}

// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassA']/method[@name='packageMethodA' and count(parameter)=0]"
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodA' and count(parameter)=0]"
[global::Java.Interop.JniMethodSignature ("packageMethodA", "()V")]
public unsafe void PackageMethodA ()
{
Expand Down
4 changes: 2 additions & 2 deletions tests/generator-Tests/expected.xaji/AccessModifiers/Xamarin.Test.PublicFinalClass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public unsafe void PublicMethod ()
}
}

// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassB']/method[@name='packageMethodB' and count(parameter)=0]"
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodB' and count(parameter)=0]"
[Register ("packageMethodB", "()V", "")]
public unsafe void PackageMethodB ()
{
Expand All @@ -68,7 +68,7 @@ public unsafe void PackageMethodB ()
}
}

// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassA']/method[@name='packageMethodA' and count(parameter)=0]"
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodA' and count(parameter)=0]"
[Register ("packageMethodA", "()V", "")]
public unsafe void PackageMethodA ()
{
Expand Down
4 changes: 2 additions & 2 deletions tests/generator-Tests/expected/AccessModifiers/Xamarin.Test.PublicFinalClass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public unsafe void PublicMethod ()
}

static IntPtr id_packageMethodB;
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassB']/method[@name='packageMethodB' and count(parameter)=0]"
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodB' and count(parameter)=0]"
[Register ("packageMethodB", "()V", "")]
public unsafe void PackageMethodB ()
{
Expand All @@ -62,7 +62,7 @@ public unsafe void PackageMethodB ()
}

static IntPtr id_packageMethodA;
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassA']/method[@name='packageMethodA' and count(parameter)=0]"
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodA' and count(parameter)=0]"
[Register ("packageMethodA", "()V", "")]
public unsafe void PackageMethodA ()
{
Expand Down
2 changes: 1 addition & 1 deletion tools/generator/Java.Interop.Tools.Generator.ObjectModel/ClassGen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public override void FixupAccessModifiers (CodeGenerationOptions opt)
foreach (var baseMethod in baseClass.Methods) {
var method = Methods.FirstOrDefault (m => m.Matches (baseMethod));
if (method == null)
Methods.Add (baseMethod);
Methods.Add (baseMethod.Clone (this));
}
BaseType = baseClass.BaseType;
} else {
Expand Down
5 changes: 5 additions & 0 deletions tools/generator/Java.Interop.Tools.Generator.ObjectModel/GenericParameterDefinition.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ public bool Validate (CodeGenerationOptions opt, GenericParameterDefinitionList
validated = is_valid = true;
return true;
}

public GenericParameterDefinition Clone ()
{
return new GenericParameterDefinition (Name, ConstraintExpressions);
}
}

public class GenericParameterDefinitionList : List<GenericParameterDefinition>
Expand Down
51 changes: 51 additions & 0 deletions tools/generator/Java.Interop.Tools.Generator.ObjectModel/Method.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,57 @@ internal string CalculateEventName (Func<string, bool> checkNameDuplicate)

public bool CanHaveStringOverload => IsReturnCharSequence || Parameters.HasCharSequence;

public Method Clone (GenBase declaringType)
{
var clone = new Method (declaringType);

// MethodBase
clone.Annotation = Annotation;
clone.ApiAvailableSince = ApiAvailableSince;
clone.AssemblyName = AssemblyName;
clone.Deprecated = Deprecated;
clone.DeprecatedSince = DeprecatedSince;
clone.IsAcw = IsAcw;
clone.Name = Name;
clone.Visibility = Visibility;
clone.LineNumber = LineNumber;
clone.LinePosition = LinePosition;
clone.SourceFile = SourceFile;
clone.JavadocInfo = JavadocInfo;

if (GenericArguments != null)
foreach (var ga in GenericArguments)
clone.GenericArguments.Add (ga.Clone ());

foreach (var p in Parameters)
clone.Parameters.Add (p.Clone ());
Copy link
Contributor

Choose a reason for hiding this comment

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

While writing the draft commit message, my thoughts turn to BaseClass<V>.doThing(V): wouldn't this still have a parameter type of V? How is that "resolved" via declaringType.TypeParameters?

Rephrased, I would expect that the clone instance will have clone.Parameters.Count==1, and clone.Parameters[0] equivalent to:

new Parameter (
        name: "value",
        type: "java.lang.Object",
        managedType: "Java.Lang.Object"
        isEnumified: false
);

but I don't see this happening. Should this be happening?

Or is this instead handled by new lines 128-130 (above), wherein we added this.GenericArguments to clone.GenericArguments. Though I'm not certain about that either, as shouldn't GenericArguments be empty as BaseClass<V>.doThing(V) isn't itself generic…


// Method
clone.ArgsType = ArgsType;
clone.CustomAttributes = CustomAttributes;
clone.EventName = EventName;
clone.GenerateAsyncWrapper = GenerateAsyncWrapper;
clone.GenerateDispatchingSetter = GenerateDispatchingSetter;
clone.IsAbstract = IsAbstract;
clone.IsFinal = IsFinal;
clone.IsInterfaceDefaultMethod = IsInterfaceDefaultMethod;
clone.OverriddenInterfaceMethod = OverriddenInterfaceMethod;
clone.IsReturnEnumified = IsReturnEnumified;
clone.IsStatic = IsStatic;
clone.IsVirtual = IsVirtual;
clone.JavaName = JavaName;
clone.ManagedOverride = ManagedOverride;
clone.ManagedReturn = ManagedReturn;
clone.PropertyNameOverride = PropertyNameOverride;
clone.Return = Return;
clone.ReturnNotNull = ReturnNotNull;
clone.RetVal = RetVal.Clone (clone);
clone.SourceApiLevel = SourceApiLevel;
clone.ExplicitInterface = ExplicitInterface;

return clone;
}

public string ConnectorName => $"Get{Name}{IDSignature}Handler";

public string EscapedCallbackName => IdentifierValidator.CreateValidIdentifier ($"cb_{JavaName}{IDSignature}", true);
Expand Down
7 changes: 6 additions & 1 deletion tools/generator/Java.Interop.Tools.Generator.ObjectModel/Parameter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ internal Parameter (string name, string type, string managedType, bool isEnumifi
this.is_enumified = isEnumified;
NotNull = notNull;
}


public Parameter Clone ()
{
return new Parameter (name, type, managed_type, is_enumified, rawtype, NotNull);
}

public string GetCall (CodeGenerationOptions opt)
{
var rgm = sym as IRequireGenericMarshal;
Expand Down
5 changes: 5 additions & 0 deletions tools/generator/Java.Interop.Tools.Generator.ObjectModel/ReturnValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ public ReturnValue (Method owner, string java_type, string managed_type, bool is

public string CallMethodPrefix => TypeNameUtilities.GetCallPrefix (sym);

public ReturnValue Clone (Method owner)
{
return new ReturnValue (owner, java_type, managed_type, is_enumified, NotNull);
}

public string DefaultValue {
get { return sym.DefaultValue; }
}
Expand Down