[Xamarin.Android.Build.Tasks] Add more warning codes

[brendanzagaeski]

Context: #1560

• I investigated the code paths that led into the LogWarning() in
  GenerateResourceDesigner.AddRename(). I could not find any way to
  hit that message with the current code. Moreover, the only reports of
  the message I could find on the web were from capitalization
  mismatches, and those are now explicitly ignored by the use of the
  StringComparison.OrdinalIgnoreCase argument in the call to
  string.Compare(). Since this message does not currently indicate an
  actionable problem, I changed it from a warning to a debug message.

• Neither of the warnings in
  GetAdditionalResourcesFromAssemblies.AddAttributeValue() is
  actionable for Xamarin.Android app developers. The first warning can
  only arise if a new attribute name has been added to the switch
  statement in DoExecute(). The second warning can only occur in a
  library that uses one of those attributes. Since the attributes are
  not formally documented, they are currently only intended for use by
  the Xamarin team. Since these warnings aren't aimed at end-users,
  they don't need to have their own codes. So for now, I just updated
  the warnings to reuse the errorCode.

• The first warning in
  GetAdditionalResourcesFromAssemblies.MakeSureLibraryIsInPlace() is
  just an informational message. The download step is expected to be
  skipped during design-time builds. I changed the warning to a debug
  message accordingly.

• I changed the warning in ReadAdditionalResourcesFromAssemblyCache to
  a debug message because it is just informational. For example, in the
  normal process of building application projects, the
  Xamarin.Android.Common.targets checks if CacheFile exists before it
  invokes the task, so an absent CacheFile is often normal. In cases
  where the warning could be actionable for an end-user, the warning
  would indicate that something went wrong earlier when creating the
  CacheFile in GetAdditionalResourcesFromAssemblies, so even in that
  case this warning would not be needed because more actionable messages
  should come from the earlier task. On top of that, this whole old
  mechanism of downloading external files is rarely used now that the
  large majority of the NuGet packages from the Xamarin team have been
  updated to use the newer Xamarin.Build.Download mechanism.

• I also changed the warnings in ReadImportedLibrariesCache and
  ReadLibraryProjectImportsCache to debug messages for similar reasons.
  A missing cache file in one of these tasks indicates that something
  went wrong during the corresponding earlier GetImportedLibraries or
  ResolveLibraryProjectImports task, so any actionable messages would
  need to get logged in the earlier task instead.

• The warnings in
  ResolveAssemblies.ResolveRuntimeAssemblyForReferenceAssembly()
  should not occur during normal use. They both depend on generated
  content related to the NuGet target framework moniker. Based on these
  considerations, I assigned just one shared warning code to both
  warnings for now.

[dellis1972]

@brendanzagaeski does the wording on one of these messages need to be flipped? eg.

Mappings between managed types and Java types must be unique
Mappings between Java types and managed types must be unique

I suspect the two checks are from the opposite point of view. So we should reword the message to reflect which point of view we were using? maybe?

[brendanzagaeski]

Yeah, ensuring clear language in the actual message that comes from the warnings is good to do. For this pass, I was only planning to update messages "opportunistically" if I saw cases where an idea came to mind immediately as I was reading them for a better wording. But I'd be happy to spend a little more time reviewing the wording as I go, if that'd be useful. For these 2 particular warnings, I also had a hunch that users might not run into them often (we'll know better once we start getting the telemetry from the warning codes). So I didn't spend too much time thinking about the wording yet.

[brendanzagaeski]

I took another look at this warning & error pair today. It occurred to me that it might be best to rephrase these messages to be more like the error that Csc gives for similar type conflicts. That led me to see that the current messages use a slightly different way of splitting up the type and assembly name compared to the Csc message, so getting everything to match up closely would take some extra work. Long story short, I took a try at updating the messages, but I'm not sure if the new wording better overall. If it does look like an improvement, I can add it to this pull request.

[jonpryor]

I think improving the wording is a good idea. I'm unsure whether the current suggestion is a net improvement.

Consider a solution with two library projects, referenced by the app project, which contain the following class:

using System;
using Android.Runtime;

namespace Lib1
{
  [Register ("examplelib.EmptyClass")]
  public class EmptyClass : Java.Lang.Object
  {
  }
}

This elicits the warning (newlines added for readability):

warning : Duplicate managed type found! Mappings between managed types and Java types must be unique.
 First Type: 'Lib1.EmptyClass, Lib1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null';
Second Type: 'Lib1.EmptyClass, Lib2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'.

Hilariously, if we update the project structure so that Lib1 has Lib1.EmptyClass and Lib2 has Lib2.EmptyClass -- instead of both Lib1 and Lib2 providing Lib1.EmptyClass -- then we get an error (newlines added for readability):

error : Duplicate Java type found! Mappings between managed types and Java types must be unique.
 First Type: 'Lib1.EmptyClass, Lib1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null';
Second Type: 'Lib2.EmptyClass, Lib2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'

I find this hilarious, as it implies one form of ambiguity is worse than another, even though either way we have ambiguity. :-/

That setup aside, if we imagine the suggested XA4214 warning message, we'd have:

The managed type 'Lib1.EmptyClass' exists as both 'Lib1.EmptyClass, Lib1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' and Lib1.EmptyClass, Lib2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'. An Android Callable Wrapper will only be generated for 'Lib1.EmptyClass, Lib1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'.

Is this better? Not immediately, no; it's not actionable. It doesn't inform why this might be a problem -- which arguably could/should instead be in the docs -- nor does it have any suggestion of how to fix it.

(The fact that we have assembly-qualified names in here also makes me think we'd be better off with multi-line messages here!)

[jonpryor]

After pondering the corresponding documentation, how's this for an improved warning message?

The managed type `Lib1.EmptyClass` exists in the assemblies `Lib1`, `Lib2`.  Please refactor the managed type names so that they are not identical.

C#-wise, this would require additional changes to <GenerateJavaStubs/> so that we can properly capture all the assemblies that contain duplicate types, instead of the current code which emits the warning as soon as it finds a duplicate, and then will re-issue the "same" warning every time.

For example, my previous examples have all had two assemblies (Lib1 & Lib2) with a Lib1.EmptyClass type. What happens when there are three assemblies with the same type? Four assemblies? 10? With the current approach, we'll emit assemblies.Length-1 warnings, which is potentially very noise and not as actionable.

[dellis1972]

Grammar. These warnings :)

[dellis1972]

I would break the previous sentence up in to two at the or. Its currently difficult to read.

[dellis1972]

should naive be native ?

[brendanzagaeski]

Good sanity-check :-) In this case as I understand it, it is indeed intended to be "naive" because the fixup approach is fairly simplistic. Context:
https://github.com/xamarin/xamarin-android/blob/14d91c314f9c9f2ba7b1d69556b6b2044afb6ab5/src/Xamarin.Android.Build.Tasks/Tasks/CalculateLayoutCodeBehind.cs#L320-L331

[jonpryor]

@brendanzagaeski: For future reference, please use separate PRs for each added warning/error code. Smaller & more frequent PRs are preferable to larger and less frequent PRs.

[brendanzagaeski]

please use separate PRs for each added warning/error code

👍 Will do.

[jonpryor]

...now that we're digging into this, an obvious question is this: Why does anybody care?

Additionally, the description is wrong.

(Also, let's stop using "Android Callable Wrapper" and stick with "Java Callable Wrapper". ;-)

Consider a variation on the previous repro instructions: two assemblies which both declare the "same" C# type:

namespace Lib1
{
  public class EmptyClass : Java.Lang.Object
  {
  }
}

We thus have two different Lib1.EmptyClass types running around. However, because there is no [Register], then we hit the default "md5-ify everything" approach, which means we can emit two separate Java Callable Wrapper types: md56bf89f81d87b2a3b578f763649ead9ef.EmptyClass for Lib1.EmptyClass, Lib1 and md551eaf02b2ef90dc8aacda5445b93be5c.EmptyClass for Lib1.EmptyClass, Lib2.

We can -- do -- generate Java Callable Wrappers for both types.

Thus, why care that we have two separate "Lib1.EmptyClass" types running around? What's the actual problem?

The actual problem comes in places where an assembly cannot be specified, such as within .axml files. There, if we used <Lib1.EmptyClass/>, there is no way to say "use the EmptyClass type from Lib1" vs. "use the EmptyClass type from Lib2". It's ambiguous.

However, fragments allow avoiding this, e.g. by using <fragment type="Lib1.EmptyClass, Lib1" />. (No idea if anyone actually uses that functionality, but it exists.) Again, though, if you use <fragment type="Lib1.EmptyClass" /> -- no assembly -- then one of the types is picked. The first one.

What is the fix? Change the type names so that they have different C# Namespace.Type names. The documentation should likewise mention how to fix the warning.

[jonpryor]

The solution should be in a new paragraph.

Regarding the suggested new error text, we'd currently emit:

error : Duplicate Java type found! Mappings between managed types and Java types must be unique. First Type: 'Lib1.EmptyClass, Lib1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'; Second Type: 'Lib2.EmptyClass, Lib2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'

In the spirit of this suggestion for XAX4214, what about:

error XA4215: The Java type `examplelib.EmptyClass` is generated by more than one managed type. Please change the [Register] attribute so that the same Java type is not emitted.
error XA4215:   `examplelib.EmptyClass` generated by: Lib1.EmptyClass, Lib1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null
error XA4215:   `examplelib.EmptyClass` generated by: Lib1.EmptyClass, Lib2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null

If we get really extravagant, we could emit the filename & line number information for the class declarations of the offending types:

path/to/Lib1/ExampleClass.cs:7: error XA4215: The Java type ...
path/to/Lib1/ExampleClass.cs:7: error XA4215:   `examplelib.EmptyClass` generated by: Lib1.EmptyClass, Lib1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null
path/to/Lib2/ExampleClass.cs:7: error XA4215:   `examplelib.EmptyClass` generated by: Lib2.EmptyClass, Lib1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null

[jonpryor]

please use separate PRs for each added warning/error code

👍 Will do.

Continuing the aside, just the back & forth over the past day suggests why "one PR per update" is better here. More than half of this PR is fine/acceptable: XA0118, XA1005, the LogDebugMessage() changes.

XA4214 and XA4215 have completely blown things up. :-)

In the interests of moving forward, could you split up this PR so it only includes the XA0118, XA1005, the LogDebugMessage() changes? We can then deal with XA4214 and XA4215 separately.

[brendanzagaeski]

I have now updated this pull request to omit the changes for XA4214 and XA4215 so that we can follow-up on those separately.

The XA4214/XA4215 errors have been split out.