Skip to content

[generator] Warn if type name matches enclosing namespace name. #1010

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
jonpryor merged 2 commits into from
Jul 14, 2022
Merged

[generator] Warn if type name matches enclosing namespace name. #1010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2af1848
[generator] Warn if type name matches enclosing namespace name.
jpobst Jul 12, 2022
146213f
Address feedback.
jpobst Jul 14, 2022
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
9 changes: 9 additions & 0 deletions src/Java.Interop.Localization/Resources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/Java.Interop.Localization/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,10 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</comment>
</data>
<data name="Generator_BG8403" xml:space="preserve">
<value>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</value>
<comment>{0} - Java type.</comment>
</data>
<data name="Generator_BG8500" xml:space="preserve">
<value>Unexpected child element of '&lt;interface&gt;': '{0}'.</value>
<comment>{0} - XML element name.
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.cs.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.de.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.es.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.fr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.it.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.ja.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.ko.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.pl.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.pt-BR.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.ru.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.tr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.zh-Hans.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Java.Interop.Localization/xlf/Resources.zh-Hant.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
{1} - .NET field name.
{2} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8403">
<source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
<target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
<note>{0} - Java type.</note>
</trans-unit>
<trans-unit id="Generator_BG8500">
<source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
<target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
Expand Down
23 changes: 19 additions & 4 deletions src/Java.Interop.Tools.Generator/Utilities/Report.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics;
using System.Xml;
using System.Xml.Linq;

Expand All @@ -7,6 +8,7 @@ namespace Java.Interop.Tools.Generator
public class Report
{
public static int? Verbosity { get; set; }
public static Action<TraceLevel, string>? OutputDelegate { get; set; }

public class LocalizedMessage
{
Expand Down Expand Up @@ -43,6 +45,7 @@ public LocalizedMessage (int code, string value)
public static LocalizedMessage WarningFieldNameCollision_Method => new LocalizedMessage (0x8401, Localization.Resources.Generator_BG8401_Method);
public static LocalizedMessage WarningFieldNameCollision_NestedType => new LocalizedMessage (0x8401, Localization.Resources.Generator_BG8401_NestedType);
public static LocalizedMessage WarningDuplicateField => new LocalizedMessage (0x8402, Localization.Resources.Generator_BG8402);
public static LocalizedMessage WarningTypeNameMatchesEnclosingNamespace => new LocalizedMessage (0x8403, Localization.Resources.Generator_BG8403);
public static LocalizedMessage WarningUnexpectedInterfaceChild => new LocalizedMessage (0x8500, Localization.Resources.Generator_BG8500);
public static LocalizedMessage WarningEmptyEventName => new LocalizedMessage (0x8501, Localization.Resources.Generator_BG8501);
public static LocalizedMessage WarningInvalidDueToInterfaces => new LocalizedMessage (0x8502, Localization.Resources.Generator_BG8502);
Expand Down Expand Up @@ -95,7 +98,7 @@ public static void LogCodedError (LocalizedMessage message, XNode? node, params

public static void LogCodedError (LocalizedMessage message, string? sourceFile, int line, int column, params string? [] args)
{
Console.Error.WriteLine (Format (true, message.Code, sourceFile, line, column, message.Value, args));
WriteOutput (TraceLevel.Error, Format (true, message.Code, sourceFile, line, column, message.Value, args));
}

public static void LogCodedWarning (int verbosity, LocalizedMessage message, params string? [] args)
Expand All @@ -120,17 +123,17 @@ public static void LogCodedWarning (int verbosity, LocalizedMessage message, Exc
return;

var supp = innerException != null ? " For details, see verbose output." : null;
Console.Error.WriteLine (Format (false, message.Code, sourceFile, line, column, message.Value, args) + supp);
WriteOutput (TraceLevel.Warning, Format (false, message.Code, sourceFile, line, column, message.Value, args) + supp);

if (innerException != null)
Console.Error.WriteLine (innerException);
WriteOutput (TraceLevel.Warning, innerException.ToString ());
}

public static void Verbose (int verbosity, string format, params object?[] args)
{
if (verbosity > (Verbosity ?? 0))
return;
Console.Error.WriteLine (format, args);
WriteOutput (TraceLevel.Verbose, format, args);
}

public static string FormatCodedMessage (bool error, LocalizedMessage message, params object? [] args)
Expand Down Expand Up @@ -169,6 +172,18 @@ public static string Format (bool error, int errorCode, string? sourceFile, int

return (file, pos?.LineNumber ?? -1, pos?.LinePosition ?? -1);
}

static void WriteOutput (TraceLevel traceLevel, string format, params object?[] args)
{
// Write to overridden output if requested
if (OutputDelegate != null) {
OutputDelegate (traceLevel, string.Format (format, args));
return;
}

// Write to Console.Error
Console.Error.WriteLine (format, args);
}
}

/// <summary>
Expand Down
44 changes: 44 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
@@ -1,7 +1,10 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;
using generator.SourceWriters;
using Java.Interop.Tools.Generator;
using MonoDroid.Generation;
using NUnit.Framework;
using Xamarin.Android.Binder;
Expand Down Expand Up @@ -527,6 +530,47 @@ public void ObsoleteBoundMethodAbstractDeclaration ()
// Ensure [Obsolete] was written
Assert.True (writer.ToString ().Contains ("[Obsolete (@\"This is so old!\")]"), writer.ToString ());
}

[Test]
[NonParallelizable] // We are setting a static property on Report
public void WarnIfTypeNameMatchesNamespace ()
{
var @class = new TestClass ("Object", "java.myclass.MyClass");
var sb = new StringBuilder ();

var write_output = new Action<TraceLevel, string> ((t, s) => { sb.AppendLine (s); });
Report.OutputDelegate = write_output;

generator.Context.ContextTypes.Push (@class);
generator.WriteType (@class, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
generator.Context.ContextTypes.Pop ();

Report.OutputDelegate = null;

// Ensure the warning was raised
Assert.True (sb.ToString ().Contains ("warning BG8403"));
}

[Test]
[NonParallelizable] // We are setting a static property on Report
public void DontWarnIfNestedTypeNameMatchesNamespace ()
{
var @class = new TestClass ("Object", "java.myclass.MyParentClass");
@class.NestedTypes.Add (new TestClass ("Object", "java.myclass.MyParentClass.MyClass"));
var sb = new StringBuilder ();

var write_output = new Action<TraceLevel, string> ((t, s) => { sb.AppendLine (s); });
Report.OutputDelegate = write_output;

generator.Context.ContextTypes.Push (@class);
generator.WriteType (@class, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
generator.Context.ContextTypes.Pop ();

Report.OutputDelegate = null;

// The warning should not be raised if the nested type matches enclosing namespace
Assert.False (sb.ToString ().Contains ("warning BG8403"));
}
}

[TestFixture]
Expand Down
45 changes: 45 additions & 0 deletions tools/generator/Extensions/StringExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
using System;
using System.Diagnostics.CodeAnalysis;

namespace generator
{
static class StringExtensions
{
/// <summary>
/// Shortcut for !string.IsNullOrWhiteSpace (s)
/// </summary>
public static bool HasValue (this string s) => !string.IsNullOrWhiteSpace (s);

/// <summary>
/// Removes the final subset of a delimited string. ("127.0.0.1" -> "127.0.0")
/// </summary>
public static string ChompLast (this string s, char separator)
{
if (!s.HasValue ())
return s;

var index = s.LastIndexOf (separator);

if (index < 0)
return string.Empty;

return s.Substring (0, index);
}

/// <summary>
/// Returns the final subset of a delimited string. ("127.0.0.1" -> "1")
/// </summary>
public static string LastSubset (this string s, char separator)
{
if (!s.HasValue ())
return s;

var index = s.LastIndexOf (separator);

if (index < 0)
return s;

return s.Substring (index + 1);
}
}
}
4 changes: 4 additions & 0 deletions tools/generator/Java.Interop.Tools.Generator.CodeGeneration/JavaInteropCodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ public override void WriteType (GenBase gen, string indent, GenerationInfo gen_i
else
throw new InvalidOperationException ("Unknown GenBase type");

// We do this here because we only want to check for top-level types,
// we should not check types nested in other types.
SourceWriterExtensions.WarnIfTypeNameMatchesNamespace (type_writer, gen);

var cw = new CodeWriter (writer, indent);
type_writer.Write (cw);
}
Expand Down
13 changes: 13 additions & 0 deletions tools/generator/SourceWriters/Extensions/SourceWriterExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,5 +397,18 @@ public static TypeWriter BuildManagedTypeModel (GenBase gen, CodeGenerationOptio

throw new InvalidOperationException ("Unknown GenBase type");
}

public static void WarnIfTypeNameMatchesNamespace (TypeWriter type, GenBase gen)
{
// We only care about the last part of a namespace
// eg: android.graphics.drawable -> drawable
var ns = gen.Namespace?.LastSubset ('.');

if (!ns.HasValue ())
return;

if (string.Equals (ns, type.Name, StringComparison.OrdinalIgnoreCase))
Report.LogCodedWarning (0, Report.WarningTypeNameMatchesEnclosingNamespace, $"{gen.Namespace}.{type.Name}");
}
}
}