Skip to content

[generator] Be smarter about when members with same names need "new". #567

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

Merged
merged 1 commit into from
Feb 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
130 changes: 130 additions & 0 deletions tests/generator-Tests/Unit-Tests/GenBaseTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using MonoDroid.Generation;
using NUnit.Framework;

namespace generatortests
{
[TestFixture]
public class GenBaseTests
{
CodeGenerationOptions options = new CodeGenerationOptions ();

[Test]
public void PropertyRequiresNew ()
{
var c = SupportTypeBuilder.CreateClassWithProperty ("MyClass", "java.myClass", "Handle", "int", options);
Assert.True (c.RequiresNew (c.Properties.First ()));

c.Properties.First ().Name = "GetHashCode";
Assert.True (c.RequiresNew (c.Properties.First ()));

c.Properties.First ().Name = "GetType";
Assert.True (c.RequiresNew (c.Properties.First ()));

c.Properties.First ().Name = "ToString";
Assert.True (c.RequiresNew (c.Properties.First ()));

c.Properties.First ().Name = "Equals";
Assert.True (c.RequiresNew (c.Properties.First ()));

c.Properties.First ().Name = "ReferenceEquals";
Assert.True (c.RequiresNew (c.Properties.First ()));

c.Properties.First ().Name = "Handle2";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why Handle2 should require a new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's Asserting that it doesn't require a new. :)

Assert.False (c.RequiresNew (c.Properties.First ()));
}

[Test]
public void ToStringRequiresNew () => TestParameterlessMethods ("ToString");

[Test]
public void GetTypeRequiresNew () => TestParameterlessMethods ("GetType");

[Test]
public void GetHashCodeRequiresNew () => TestParameterlessMethods ("GetHashCode");

[Test]
public void StaticEqualsCodeRequiresNew () => TestStaticMethodsWithTwoParameters ("Equals");

[Test]
public void ReferenceEqualsEqualsCodeRequiresNew () => TestStaticMethodsWithTwoParameters ("ReferenceEquals");

[Test]
public void HandleAlwaysRequiresNew ()
{
// The same name as a property always requires new, no matter the parameters
var c = SupportTypeBuilder.CreateClass ("java.myClass", options);
var m = SupportTypeBuilder.CreateMethod (c, "Handle", options);

// Yes
Assert.True (c.RequiresNew (m.Name, m));

// Yes, even with parameters
m.Parameters.Add (new Parameter ("value", "int", "int", false));

Assert.True (c.RequiresNew (m.Name, m));
}

[Test]
public void TestEqualsMethodsWithOneParameter ()
{
var c = SupportTypeBuilder.CreateClass ("java.myClass", options);
var m = SupportTypeBuilder.CreateMethod (c, "Equals", options, "void", false, false);

// No because 0 parameters
Assert.False (c.RequiresNew (m.Name, m));

var p0 = new Parameter ("p0", "object", "object", false);
m.Parameters.Add (p0);

// Yes
Assert.True (c.RequiresNew (m.Name, m));

// No because parameter is wrong type
var p1 = new Parameter ("p1", "string", "string", false);
m = SupportTypeBuilder.CreateMethod (c, "Equals", options, "void", true, false, p1);

Assert.False (c.RequiresNew (m.Name, m));
}

void TestParameterlessMethods (string name)
{
var c = SupportTypeBuilder.CreateClass ("java.myClass", options);
var m = SupportTypeBuilder.CreateMethod (c, name, options);

// Yes
Assert.True (c.RequiresNew (m.Name, m));

// No because > 0 parameters
m.Parameters.Add (new Parameter ("value", "int", "int", false));

Assert.False (c.RequiresNew (m.Name, m));
}

void TestStaticMethodsWithTwoParameters (string name)
{
var c = SupportTypeBuilder.CreateClass ("java.myClass", options);

var p0 = new Parameter ("p0", "object", "object", false);
var p1 = new Parameter ("p1", "object", "object", false);
var m = SupportTypeBuilder.CreateMethod (c, name, options, "void", true, false, p0, p1);

// Yes
Assert.True (c.RequiresNew (m.Name, m));

// No because != 2 parameters
m.Parameters.Add (new Parameter ("value", "int", "int", false));
Assert.False (c.RequiresNew (m.Name, m));

// No because parameter is wrong type
var p2 = new Parameter ("p1", "string", "string", false);
m = SupportTypeBuilder.CreateMethod (c, name, options, "void", true, false, p0, p2);

Assert.False (c.RequiresNew (m.Name, m));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1119,7 +1119,7 @@ public void WriteMethodAbstractDeclaration (Method method, string indent, Interf
WriteMethodCustomAttributes (method, indent);
writer.WriteLine ("{0}{1}{2} abstract {3} {4} ({5});",
indent,
impl.RequiresNew (method.Name) ? "new " : "",
impl.RequiresNew (method.Name, method) ? "new " : "",
method.Visibility,
opt.GetOutputName (method.RetVal.FullName),
name,
Expand Down Expand Up @@ -1367,7 +1367,7 @@ public void WriteMethod (Method method, string indent, GenBase type, bool genera
seal = string.Empty;
}

if ((string.IsNullOrEmpty (virt_ov) || virt_ov == " virtual") && type.RequiresNew (method.AdjustedName)) {
if ((string.IsNullOrEmpty (virt_ov) || virt_ov == " virtual") && type.RequiresNew (method.AdjustedName, method)) {
virt_ov = " new" + virt_ov;
}
string ret = opt.GetOutputName (method.RetVal.FullName);
Expand Down Expand Up @@ -1460,7 +1460,7 @@ public void WriteProperty (Property property, GenBase gen, string indent, bool w
force_override = true;

string decl_name = property.AdjustedName;
string needNew = gen.RequiresNew (decl_name) ? " new" : "";
string needNew = gen.RequiresNew (property) ? " new" : "";
string virtual_override = String.Empty;
bool is_virtual = property.Getter.IsVirtual && (property.Setter == null || property.Setter.IsVirtual);
if (with_callbacks && is_virtual) {
Expand Down Expand Up @@ -1537,7 +1537,7 @@ public void WritePropertyAbstractDeclaration (Property property, string indent,
string abstract_name = property.AdjustedName;
string visibility = property.Getter.RetVal.IsGeneric ? "protected" : property.Getter.Visibility;
if (!overrides) {
requiresNew = gen.RequiresNew (abstract_name);
requiresNew = gen.RequiresNew (property);
WritePropertyCallbacks (property, indent, gen, abstract_name);
}
writer.WriteLine ("{0}{1}{2} abstract{3} {4} {5} {{",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -727,10 +727,44 @@ public string [] PreCallback (CodeGenerationOptions opt, string var_name, bool o

public string RawVisibility => support.Visibility;

public bool RequiresNew (string memberName)
public bool RequiresNew (Property property)
{
switch (property.AdjustedName.ToLowerInvariant ()) {
case "handle":
case "gethashcode":
case "gettype":
case "tostring":
case "equals":
case "referenceequals":
return true;
}

return IsThrowable () && ThrowableRequiresNew.Contains (property.AdjustedName);
}

public bool RequiresNew (string memberName, Method method)
{
if (ObjectRequiresNew.Contains (memberName))
return true;
switch (memberName.ToLowerInvariant ()) {
case "handle":
// The same name as a property always requires new, no matter the parameters
return true;
case "gethashcode":
case "gettype":
case "tostring":
return method.Parameters.Count == 0;
case "equals":
if (method.Parameters.Count == 1 && method.Parameters.All (p => p.Type == "object"))
return true;
if (method.Parameters.Count == 2 && method.Parameters.All (p => p.Type == "object"))
return true;

break;
case "referenceequals":
if (method.Parameters.Count == 2 && method.Parameters.All (p => p.Type == "object"))
return true;

break;
}

return IsThrowable () && ThrowableRequiresNew.Contains (memberName);
}
Expand Down