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

[One .NET] support latest C# 10 language features #6118

Merged

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jul 23, 2021

Context: dotnet/sdk#19521
Fixes: #6075
Fixes: #6076

We need to make two sets of changes for C# 10:

  1. Support "global usings". Our .NET 6 templates should have no
    using statements at the top of .cs files.
  2. Use $(Nullable) enable by default in project templates.

To test this, our .NET 6 MSBuild tests use Nullable=enable and
ImplicitUsings=enable by default and do not include using
statements in .cs files.

I've made a new MainActivity.cs for our .NET 6 MSBuild tests. The
"legacy" Xamarin.Android tests will use the original file.

Our default global using are:

global using global::Android.App;
global using global::Android.Widget;
global using Bundle = global::Android.OS.Bundle;

The last one is intentionally not bringing in Android.OS, because
Android.OS.Environment would conflict with System.Environment.

So AutoImport.props should become:

<ItemGroup Condition=" '$(TargetPlatformIdentifier)' == 'android' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
  <Using Include="Android.App" />
  <Using Include="Android.Widget" />
  <Using Include="Android.OS.Bundle" Alias="Bundle" />
</ItemGroup>

So these items are present at the time .csproj files are evaluated.

Any templates will add:

<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>

If users want to configure these settings, they can remove
$(ImplicitUsings) from the .csproj completely or remove specific
@(Using) items:

<ItemGroup>
  <Using Remove="Android.App" />
</ItemGroup>

@jonathanpeppers
Copy link
Member Author

This is a draft, because I suspect we might get test failures on this one.

@jonathanpeppers jonathanpeppers changed the title [One .NET] support latest C #10 language features [One .NET] support latest C# 10 language features Jul 23, 2021
@jonathanpeppers
Copy link
Member Author

One complication that came up in a test:

error CS0104: 'Application' is an ambiguous reference between 'Android.App.Application' and 'Xamarin.Forms.Application'

So I suspect that dotnet/maui is going to set $(DisableImplicitNamespaceImports_Android). Because it has to? There are so many types that will conflict.

@jonathanpeppers jonathanpeppers force-pushed the dotnet-csharp-10-language-features branch from e628acd to 245ec40 Compare July 23, 2021 21:15
@jonpryor
Copy link
Member

@jonathanpeppers wrote:

One complication that came up in a test:

error CS0104: 'Application' is an ambiguous reference between 'Android.App.Application' and 'Xamarin.Forms.Application'

This suggests that we might not want Android.App in the default @(Import) either, since lots of our customers will be using MAUI.

Will Android.Widget conflict with anything?

The "better answer" here may be to just not add anything to @(Import), as anything we add will likely conflict with MAUI…

jonathanpeppers added a commit to xamarin/Xamarin.Legacy.Sdk that referenced this pull request Jul 26, 2021
Context: dotnet/android#6118

Running into issues with C# 10 global usings, because Xamarin.Legacy.Sdk is building with C# 8.0 by default.
@jonathanpeppers jonathanpeppers force-pushed the dotnet-csharp-10-language-features branch from 245ec40 to f7a5e1d Compare July 26, 2021 20:52
@jonathanpeppers jonathanpeppers force-pushed the dotnet-csharp-10-language-features branch 2 times, most recently from fdd34a1 to 8004dd1 Compare August 4, 2021 19:38
Comment on lines 25 to 26
"assemblies/UnnamedProject.dll": {
"Size": 3171
"Size": 3535
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding Nullable=enable, increased this assembly's size enough to fail the test...

Error: apkdiff: File 'assemblies/UnnamedProject.dll' has changed by 358 bytes (10.14 %). This exceeds the threshold of 5.00 %.

@jonathanpeppers
Copy link
Member Author

We need to wait on this, because there are changes to the .NET SDK coming:

  1. @(Import) will be renamed to @(Using)
  2. $(DisableImplicitNamespaceImports)=false will be $(ImplicitUsings)=true. Projects will have to opt into it, so we will put $(ImplicitUsings)=true in our project templates.
  3. They might support our global using Bundle = global::Android.OS.Bundle; use case.

See: dotnet/sdk#19535 (comment)

I'd rather wait for the final implementation to land before we opt into the changes here.

@mhutch
Copy link
Member

mhutch commented Aug 11, 2021

The "better answer" here may be to just not add anything to @(Import), as anything we add will likely conflict with MAUI…

The MAUI targets can remove the Android global usings, or make aliasing choices.

@jonathanpeppers jonathanpeppers force-pushed the dotnet-csharp-10-language-features branch 2 times, most recently from 1064f4f to 2443637 Compare August 17, 2021 21:38
@jonathanpeppers jonathanpeppers force-pushed the dotnet-csharp-10-language-features branch 3 times, most recently from 43e480a to 9e96e08 Compare August 20, 2021 13:56
proj.MainActivity = proj.DefaultMainActivity.Replace ("//${AFTER_ONCREATE}",
proj.MainActivity = proj.DefaultMainActivity.Replace ("//${AFTER_FORMS_INIT}",
Copy link
Member Author

Choose a reason for hiding this comment

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

I found this test was not using a Forms-based MainActivity.cs before, but it was a Xamarin.Forms app! So we need to use ${AFTER_FORMS_INIT} now and the line number changed.

@jonathanpeppers jonathanpeppers marked this pull request as ready for review August 20, 2021 20:55
@jonathanpeppers jonathanpeppers force-pushed the dotnet-csharp-10-language-features branch from 9e96e08 to e42680b Compare August 23, 2021 18:00
Context: dotnet/sdk#19521
Fixes: dotnet#6075
Fixes: dotnet#6076

We need to make two sets of changes for C# 10:

1. Support "global usings". Our .NET 6 templates should have no
   `using` statements at the top of `.cs` files.
2. Use `$(Nullable)` `enable` by default in project templates.

To test this, our .NET 6 MSBuild tests use `Nullable=enable` and
`ImplicitUsings=enable` by default and do not include `using`
statements in `.cs` files.

I've made a new `MainActivity.cs` for our .NET 6 MSBuild tests. The
"legacy" Xamarin.Android tests will use the original file.

Our default `global using` are:

    global using global::Android.App;
    global using global::Android.Widget;
    global using Bundle = global::Android.OS.Bundle;

The last one is intentionally not bringing in `Android.OS`, because
`Android.OS.Environment` would conflict with `System.Environment`.

So `AutoImport.props` should become:

    <ItemGroup Condition=" '$(TargetPlatformIdentifier)' == 'android' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
      <Using Include="Android.App" />
      <Using Include="Android.Widget" />
      <Using Include="Android.OS.Bundle" Alias="Bundle" />
    </ItemGroup>

So these items are present at the time `.csproj` files are evaluated.

Any templates will add:

    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>

If users want to configure these settings, they can remove
`$(ImplicitUsings)` from the `.csproj` completely or remove specific
`@(Using)` items:

    <ItemGroup>
      <Using Remove="Android.App" />
    </ItemGroup>
@jonathanpeppers jonathanpeppers force-pushed the dotnet-csharp-10-language-features branch from e42680b to 218a708 Compare August 23, 2021 21:35
@@ -3,5 +3,7 @@
<TargetFramework>net6.0-android</TargetFramework>
<SupportedOSPlatformVersion>SUPPORTED_OS_PLATFORM_VERSION</SupportedOSPlatformVersion>
<RootNamespace Condition="'$(name)' != '$(name{-VALUE-FORMS-}safe_namespace)'">AndroidBinding1</RootNamespace>
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
Copy link
Member

Choose a reason for hiding this comment

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

Should we update AndroidBinding1.csproj to also set $(AndroidGenerateResourceDesigner)=False, as per #6224?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can have an .aar with resources, then you might want to use Resource.designer.cs values from C#.

So I don't think we need this by default, but it's there if you need to turn it off.

@jonpryor jonpryor merged commit 7fb7c81 into dotnet:main Aug 25, 2021
@jonathanpeppers jonathanpeppers deleted the dotnet-csharp-10-language-features branch August 25, 2021 17:01
jonathanpeppers added a commit that referenced this pull request Aug 25, 2021
Fixes: #6075
Fixes: #6076

Context: dotnet/sdk#19521

We need to make two sets of changes for C# 10:

 1. Support ["global usings"][0]. Our .NET 6 templates should have no
    `using` statements at the top of `.cs` files.

 2. Set `$(Nullable)`=enable by default in project templates, i.e.
    enable [C#8 nullable reference types][1] by default.

To test this, our .NET 6 MSBuild tests use `$(Nullable)`=enable and
`$(ImplicitUsings)`=enable by default and do not include `using`
statements in `.cs` files.

I've made a new `MainActivity.cs` for our .NET 6 MSBuild tests.
The "legacy" Xamarin.Android tests will use the original file.

Our default `global using` are:

	global using global::Android.App;
	global using global::Android.Widget;
	global using Bundle = global::Android.OS.Bundle;

The last one is intentionally not bringing in `Android.OS`, because
`Android.OS.Environment` would conflict with `System.Environment`,
and the `System` namespace will be in `@(Using)` by default.

`AutoImport.props` should become:

	<ItemGroup Condition=" '$(TargetPlatformIdentifier)' == 'android' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
	  <Using Include="Android.App" />
	  <Using Include="Android.Widget" />
	  <Using Include="Android.OS.Bundle" Alias="Bundle" />
	</ItemGroup>

so that these `using`s are present at the time `.csproj` files are
compiled.

Any templates will add:

	<Nullable>enable</Nullable>
	<ImplicitUsings>enable</ImplicitUsings>

If users want to configure these settings, they can remove
`$(ImplicitUsings)` from the `.csproj` completely, or remove specific
`@(Using)` items:

	<ItemGroup>
	  <Using Remove="Android.App" />
	</ItemGroup>

[0]: https://github.com/dotnet/csharplang/blob/b89d4c934041db923f7238b1427cd5f3ae71ed4b/proposals/csharp-10.0/GlobalUsingDirective.md#global-using-alias-directives
[1]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 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.

Android project templates use latest C# language idioms Add Android default global usings in MSBuild targets
3 participants