[Xamarin.Android.Build.Tasks] Add warning & error codes to GenerateJavaStubs #2349
Conversation
brendanzagaeski
requested review from
dellis1972,
jonathanpeppers and
jonpryor
as code owners
Context: dotnet/android#2349 This commit introduces a new extension method `GetAssemblyName()` that provides the assembly name portion of `GetPartialAssemblyQualifiedName()` so that `GenerateJavaStubs` doesn't have to do any string splitting to get the assembly name for warning XA4214.
|
As currently written, this pull request will require dotnet/java-interop#390 to be merged and bumped into |
|
The macOS PR Build failed to build: |
brendanzagaeski
force-pushed
the
warning-codes-2
branch
from
df6a9ad to
48a7fd1
Compare
Ah yes, whoops. I meant (and forgot) to look up whether there was a |
|
Ah nope, it looks like it's still trying to build the PR automatically. Maybe I would have had to include |
Context: dotnet/android#2349 This commit introduces a new extension method `GetPartialAssemblyName()` that provides just the assembly name portion of `GetPartialAssemblyQualifiedName()` so that `GenerateJavaStubs` doesn't need to do any string splitting to get the assembly name for warning XA4214. It also makes a small replacement in `JavaNativeTypeManager` to take advantage of the new extension method for readability. This new extension method returns a partial assembly name that is specifically just the *first* part of a fully qualified assembly name. The documentation for [System.Reflection.AssemblyName][0] and [`Type.GetType()`][1] refers to this first part as the *simple name* of the assembly. Another detail about terminology that might be useful to mention is that a type name with no qualifiers is called the *simple name* of the type, while a type name qualified with either a partial or full assembly name is called *fully qualified*. So for example, the `GetPartialAssemblyQualifiedName()` extension method returns a fully qualified type name, even though it is only qualified with a partial assembly name. [0]: https://docs.microsoft.com/dotnet/api/system.reflection.assemblyname?view=netframework-4.7.2#remarks [1]: https://docs.microsoft.com/dotnet/api/system.type.gettype?view=netframework-4.7.2#remarks
…#390) Context: dotnet/android#2349 This commit introduces a new `GetPartialAssemblyName()` extension method on `TypeReference` which provides just the assembly name portion of `GetPartialAssemblyQualifiedName()` so that `GenerateJavaStubs` doesn't need to do any string splitting to get the assembly name for warning XA4214. It also makes a small replacement in `JavaNativeTypeManager` to take advantage of the new extension method for readability. This new extension method returns a partial assembly name that is specifically just the *first* part of a fully qualified assembly name. The documentation for [`System.Reflection.AssemblyName`][0] and [`Type.GetType()`][1] refers to this first part as the *simple name* of the assembly. Another detail about terminology that might be useful to mention is that a type name with no qualifiers is called the *simple name* of the type, while a type name qualified with either a partial or full assembly name is called *fully qualified*. For example, the `GetPartialAssemblyQualifiedName()` extension method returns a fully qualified type name, even though it is only qualified with a partial assembly name. [0]: https://docs.microsoft.com/dotnet/api/system.reflection.assemblyname?view=netframework-4.7.2#remarks [1]: https://docs.microsoft.com/dotnet/api/system.type.gettype?view=netframework-4.7.2#remarks
brendanzagaeski
force-pushed
the
warning-codes-2
branch
from
48a7fd1 to
fb68810
Compare
brendanzagaeski
force-pushed
the
warning-codes-2
branch
from
fb68810 to
0f85d83
Compare
| Log.LogCodedWarning ( | ||
| "XA4214", | ||
| "The managed type `{0}` exists in more than one assembly: {1}. " + | ||
| "Please refactor the managed type names in these assemblies so that they are not identical.", |
There was a problem hiding this comment.
The current message mentions what will be used:
References to the type `Foo.Bar` will refer to `Foo.Bar, Baz`.
This information is lost by this PR. It should be preserved.
This might be a scenario where we want multiple warnings for the "same" issue:
warning XA4214: The managed type `Foo.Bar` exists in more than one assembly: Baz, Qux, Quux.
warning XA4214: References to the type `Foo.Bar` will refer to `Foo.Bar, Baz`.
| foreach (var kvp in managedConflicts) { | ||
| Log.LogCodedWarning ( | ||
| "XA4214", | ||
| "The managed type `{0}` exists in more than one assembly: {1}. " + |
There was a problem hiding this comment.
Grammar: there's more than one assembly, so we should pluralize accordingly:
warning XA4214: The managed type `Foo.Bar` is defined in the assemblies: Baz, Qux, Quux.
…vaStubs Context: dotnet#1560 Background discussion about the new wording for this particular warning and error: * dotnet#2258 (comment) * dotnet#2258 (comment) The new wording aims to be more actionable. It is also more compact than the old wording when there are multiple conflicts. The `Dictionary` initializations follow the style used in [c362fe5][0]. Since these 2 new dictionaries should usually be empty, they are set to an initial capacity of 0. The current implementation of `Dictionary` defaults to an initial capacity of 0, but the [documentation][1] doesn't mention 0 specifically, so it seems reasonable to specify it. Another idea would be to initialize each dictionary only when a corresponding conflict is found, but that might make the code trickier to read. A possible future enhancement could be to move the warning into the `ConvertCustomView` task so that it only appears when a managed type name from an Android resource file matches more than one line in the acw-map. That way, if a user wants to solve the issue by using `[Register]` attributes or assembly-qualified type names rather than renaming the managed types, the build process will automatically stop showing the warning. [0]: dotnet@c362fe5 [1]: https://docs.microsoft.com/dotnet/api/system.collections.generic.dictionary-2.-ctor
brendanzagaeski
force-pushed
the
warning-codes-2
branch
from
0f85d83 to
bda9244
Compare
|
Changes in the latest edit: Rebase on Address the requested wording changes. I'm proposing "multiple assemblies" instead of "the assemblies" to give users a small additional indication that the warning wouldn't happen if the type name only existed in one assembly. I'm happy to change it back to "the" if this seems silly. Make the included tests a little stricter by having them check for inclusion of the type name and assembly names in the warning and error messages. |
|
build |
4 participants
Context: #1560
Background discussion about the new wording for this particular warning
and error:
The new wording aims to be more actionable. It is also more compact
than the old wording when there are multiple conflicts.
A possible future enhancement could be to move the warning into the
ConvertCustomViewtask so that it only appears when a managed typename from an Android resource file matches more than one line in the
acw-map. That way, if a user wants to solve the issue by using
[Register]attributes or assembly-qualified type names rather thanrenaming the managed types, the build process will automatically stop
showing the warning.
This pull request also includes a fix from Jonathan Peppers for
GenerateJavaStubs. That fix is not strictly required for the changesto the warning and error, but it is included here because it touches
neighboring source code.
Fixes: #2266