Skip to content
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

[LayoutBindings] LayoutBindings: 'PreserveAttribute' is obsolete warnings #8529

Merged
merged 8 commits into from
Mar 27, 2024

Conversation

dellis1972
Copy link
Contributor

Fixes #7480

  • Remove the use of PreserveAttribute as we no longer support Xamarin Classic in main.
  • Add #pragma and code comments to fix certain CS* warnings which are emitted by the C# compiler.
  • Ignore a bunch of other warnings in the unit test.

@dellis1972 dellis1972 marked this pull request as ready for review November 24, 2023 21:45
void WriteDisableWarnings (State state)
{
state.WriteLine ("#pragma warning disable CS0618");
state.WriteLine ("#pragma warning disable CS8981");
Copy link
Member

Choose a reason for hiding this comment

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

CS8981 is presumably because we're emitting types based on filenames? Or is that for some other reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because some of the fields which we might auto generate might be all lowercase, the warning comes up as a result. for example having a property called settings because you have layout called settings.xml will raise this warning. I don't think we should be raising any warnings from generated code.

@jonpryor
Copy link
Member

CS1591 disabled warnings around XML doc comments.

@jonpryor
Copy link
Member

Draft commit message:

Fixes: https://github.com/xamarin/xamarin-android/issues/7480

Context: e60483341c2243a4f07a5b1c919a14f0655b7575

When [Layout Bindings][0] are enabled, a set of `partial` classes
will be generated which "mirrors" the `.axml` structure in C#.

Consider:

	dotnet new android -n com.example.codebehind

You enable Code-Behind by:

 1. Setting the `$(AndroidGenerateLayoutBindings)` MSBuild property
    to `true`.
 2. Using `android:id` on elements within `.axml` files

Consider this patch to the above `dotnet new`:

	diff --git a/Resources/layout/activity_main.xml b/Resources/layout/activity_main.xml
	index f949852..c74521c 100644
	--- a/Resources/layout/activity_main.xml
	+++ b/Resources/layout/activity_main.xml
	@@ -8,6 +8,7 @@
	         android:layout_width="wrap_content"
	         android:layout_height="wrap_content"
	         android:layout_centerInParent="true"
	+        android:id="@+id/text"
	         android:text="@string/app_text"
	     />
	 </RelativeLayout>
	diff --git a/com.example.codebehind.csproj b/com.example.codebehind.csproj
	index 3fdbcb5..8190f84 100644
	--- a/com.example.codebehind.csproj
	+++ b/com.example.codebehind.csproj
	@@ -8,5 +8,6 @@
	     <ApplicationId>com.companyname.com.example.codebehind</ApplicationId>
	     <ApplicationVersion>1</ApplicationVersion>
	     <ApplicationDisplayVersion>1.0</ApplicationDisplayVersion>
	+    <AndroidGenerateLayoutBindings>true</AndroidGenerateLayoutBindings>
	   </PropertyGroup>
	 </Project>

The resulting output contains
`$(IntermediateOutputPath)codebehind\Binding.activity_main.g.cs`,
which declares a type based on the `.axml` file name, and a mirror of
the structure:

	namespace Binding {
	    // typename based on `activity_main.xml` filename
	    sealed partial class activity_main : global::Xamarin.Android.Design.LayoutBinding {
	        [global::Android.Runtime.PreserveAttribute (Conditional=true)]
	        public activity_main (Activity client, …);
		// …

	        // `text` is via `android:id="@+id/text"`
		public TextView text => …;
	    }
	}

The problem is the use of `PreserveAttribute` in the generated code;
it's been `[Obsolete]` since e6048334 (over two years ago), which
means all projects using Layout Bindings and Layout Code-Behind always
have [CS0618][1] warnings.

Remove the emission of `PreserveAttribute, so that CS0618 is no
longer generated.

Additionally, within the generated files disable the [CS1591][2] and
[CS8981][3] warnings:

  * CS1591 is around missing XML documentation comments.  These code
    behind files do not have XML documentation comments, so if/when
    `$(DocumentationFile)` is set, these will be emitted for the
    generated code.  We do not currently plan on emitting XML
    documentation comments, so disable this warning.

 * CS8981 is emitted when a type name consists solely of lowercase
   characters.  As the generated Binding types are based on the
   filename -- which is frequently all lowercase -- this warning
   may be emitted.

[0]: https://github.com/xamarin/xamarin-android/blob/2f4e01ec15102dd9cd922cbd833f6482d69512b5/Documentation/guides/LayoutCodeBehind.md
[1]: https://learn.microsoft.com/dotnet/csharp/language-reference/compiler-messages/cs0618
[2]: https://learn.microsoft.com/dotnet/csharp/language-reference/compiler-messages/cs1591
[3]: https://learn.microsoft.com/dotnet/csharp/language-reference/compiler-messages/warning-waves#cs8981---the-type-name-only-contains-lower-cased-ascii-characters

@jonpryor
Copy link
Member

Enabling Layout Bindings still results in lots of warnings, unrelated to the warnings otherwise mentioned in this PR:

…/LayoutBinding.cs(16,98): warning CS8625: Cannot convert null literal to non-nullable reference type.
…/LayoutBinding.cs(22,90): warning CS8625: Cannot convert null literal to non-nullable reference type.
…/LayoutBinding.cs(16,13): warning CS8618: Non-nullable field 'boundView' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.
…/LayoutBinding.cs(22,13): warning CS8618: Non-nullable field 'boundActivity' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.
…/LayoutBinding.cs(35,11): warning CS8600: Converting null literal or possible null value to non-nullable type.
…/LayoutBinding.cs(37,11): warning CS8600: Converting null literal or possible null value to non-nullable type.
…/LayoutBinding.cs(79,56): warning CS8602: Dereference of a possibly null reference.
…/LayoutBinding.cs(79,56): warning CS8603: Possible null reference return.
…/LayoutBinding.cs(79,56): warning CA1422: This call site is reachable on: 'Android' 21.0 and later. 'Activity.FragmentManager' is obsoleted on: 'Android' 28.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1422)
…/LayoutBinding.cs(79,56): warning CA1422: This call site is reachable on: 'Android' 21.0 and later. 'FragmentManager.FindFragmentById<T>(int)' is obsoleted on: 'Android' 28.0 and later (This class is obsoleted in this android platform). (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1422)

Should we address these warnings within this PR? Or fix them separately?

Also, how do we fix those CA1422 warnings?

@dellis1972
Copy link
Contributor Author

We should address these in this PR I think.

…ings

Fixes dotnet#7480

* Remove the use of `PreserveAttribute` as we no longer support Xamarin
  Classic in main.
* Add #pragma and code comments to fix certain CS* warnings which are
  emitted by the C# compiler.
* Ignore a bunch of other warnings in the unit test.
@jonpryor
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Context: dotnet#8529 (comment)

When building a project that uses layout bindings, multiple nullability
warnings are emitted from `LayoutBinding.cs`:

	…/LayoutBinding.cs(16,98): warning CS8625: Cannot convert null literal to non-nullable reference type.
	…/LayoutBinding.cs(22,90): warning CS8625: Cannot convert null literal to non-nullable reference type.
	…/LayoutBinding.cs(16,13): warning CS8618: Non-nullable field 'boundView' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.
	…/LayoutBinding.cs(22,13): warning CS8618: Non-nullable field 'boundActivity' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.
	…/LayoutBinding.cs(35,11): warning CS8600: Converting null literal or possible null value to non-nullable type.
	…/LayoutBinding.cs(37,11): warning CS8600: Converting null literal or possible null value to non-nullable type.
	…/LayoutBinding.cs(79,56): warning CS8602: Dereference of a possibly null reference.
	…/LayoutBinding.cs(79,56): warning CS8603: Possible null reference return.
	…/LayoutBinding.cs(79,56): warning CA1422: This call site is reachable on: 'Android' 21.0 and later. 'Activity.FragmentManager' is obsoleted on: 'Android' 28.0 and later. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1422)
	…/LayoutBinding.cs(79,56): warning CA1422: This call site is reachable on: 'Android' 21.0 and later. 'FragmentManager.FindFragmentById<T>(int)' is obsoleted on: 'Android' 28.0 and later (This class is obsoleted in this android platform). (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1422)

Update `LayoutBinding.cs` to include nullability annotations.
Now, no warnings are emitted when
`$(AndroidGenerateLayoutBindings)`=true.
@jonathanpeppers
Copy link
Member

jonathanpeppers commented Mar 19, 2024

This will probably conflict with #8805, I fixed some trimming warnings in the same file.

(no worries, which ever is merged first is fine)

The `SuccessfulAndroidXApp` test was failing, as it asserted no
warnings, yet it had warnings:

    …\LayoutBinding.cs(61,56): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(61,72): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(79,76): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(79,95): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(88,86): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(88,105): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(16,74): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(22,66): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(12,11): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(13,7): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(14,30): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
    …\LayoutBinding.cs(33,5): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.

Slap on a `#nullable enable` so that we can reliably use
nullable reference types within `LayoutBinding.cs`.
@jonpryor jonpryor merged commit 751eb96 into dotnet:main Mar 27, 2024
47 checks passed
@dellis1972 dellis1972 deleted the Issue7480 branch March 27, 2024 18:53
grendello added a commit that referenced this pull request Mar 28, 2024
* main:
  [LayoutBindings] Fix '[Preserve]' is obsolete warnings (#8529)
  LEGO: Merge pull request 8837
grendello added a commit that referenced this pull request Apr 2, 2024
* main:
  Bump to dotnet/installer@dc43d363d2 9.0.100-preview.4.24175.5 (#8828)
  [Xamarin.Android.Build.Tasks] Update to newer ILRepack which supports debug files. (#8839)
  Bump 'NuGet.*' and 'Newtonsoft.Json' NuGet versions. (#8825)
  Localized file check-in by OneLocBuild Task (#8844)
  [LayoutBindings] Fix '[Preserve]' is obsolete warnings (#8529)
  LEGO: Merge pull request 8837
grendello added a commit that referenced this pull request Apr 3, 2024
* main:
  Bump to dotnet/installer@dc43d363d2 9.0.100-preview.4.24175.5 (#8828)
  [Xamarin.Android.Build.Tasks] Update to newer ILRepack which supports debug files. (#8839)
  Bump 'NuGet.*' and 'Newtonsoft.Json' NuGet versions. (#8825)
  Localized file check-in by OneLocBuild Task (#8844)
  [LayoutBindings] Fix '[Preserve]' is obsolete warnings (#8529)
  LEGO: Merge pull request 8837
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LayoutBindings: 'PreserveAttribute' is obsolete warnings
4 participants