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

Update implicit global usings feature to address issues #19521

Closed
12 tasks done
DamianEdwards opened this issue Aug 5, 2021 · 45 comments
Closed
12 tasks done

Update implicit global usings feature to address issues #19521

DamianEdwards opened this issue Aug 5, 2021 · 45 comments
Assignees
Milestone

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Aug 5, 2021

The new implicit global usings feature added to the SDK in 6.0.100-preview.7 has caused a few issues in some important scenarios such that some changes will be made to mitigate them and generally improve its usability.

These changes are targeting 6.0.100-rc.1

Items:

  • Rename the MSBuild property from <DisableImplicitNamespaceImports> to <ImplicitUsings>
    • Note this is for C# only
    • Visual Basic will continue to use the <DisableImplicitNamespaceImports> property for disabling implicit namespace imports
  • Add the <ImplicitUsings> property to the schema file so it appears in statement completion
  • Change the defined conditions to only enable adding the implicit usings from the SDK when the <ImplictUsings> property is set to "true" or "enable"
    • The existing conditions based on TFM should be removed
    • The only condition for adding the implicit usings as defined in the SDK will be the check that <ImplicitUsings> is equal to "true" or "enable"
  • Rename the item type that specifies the global namespaces to emit to the generated .cs file from <Import> to <Using>
    • Note this is for C# only
    • Visual Basic will continue to use the <Import> item type for specifying namespaces to import
  • Update schema for ImplicitUsings feature msbuild#6745
  • Ensure the generated file is still emitted if any <Using> items are declared (even if <ImplicitUsings> is false)
  • Ensure that implicit <Import> items for Visual Basic are not changed from what they were in .NET 5
  • Update processing of <Using> items to support defining using [alias] and using static [type] as well as using [namespace], e.g.:
    • <Using Include="Microsoft.AspNetCore.Http.Results" Alias="Results" />
      emits global using Results = global::Microsoft.AspNetCore.Http.Results;
    • <Using Include="Microsoft.AspNetCore.Http.Results" Static="True" />
      emits global using static global::Microsoft.AspNetCore.Http.Results;
    • <Using Include="Microsoft.AspNetCore.Http" />
      emits global using global::Microsoft.AspNetCore.Http;
  • Update the C# project templates to enable implicit usings for new .NET 6 projects, i.e. include <ImplicitUsings>enable</ImplicitUsings> in the .csproj file

Example project file content after these changes:

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>

  <ItemGroup>
    <Using Include="My.Awesome.Namespace" />
  </ItemGroup>
</Project>
@alexrp
Copy link

alexrp commented Aug 6, 2021

Could global using static support also be included?

@DamianEdwards
Copy link
Member Author

@alexrp yes that was the intent but I didn't capture it properly in the item RE using alias support. Updated to make it clearer.

@dougbu
Copy link
Member

dougbu commented Aug 6, 2021

nit: Suggest avoiding any parsing of the <Using /> item spec e.g.

<Using Include="Microsoft.AspNetCore.Http.Results" Alias="Results" />

and

<Using Include="Microsoft.AspNetCore.Results" Static="true" />

@DamianEdwards
Copy link
Member Author

DamianEdwards commented Aug 6, 2021

Thanks @dougbu I had the exact same thought last night and glad others agree. Will update the description.

DamianEdwards added a commit to dotnet/aspnetcore that referenced this issue Aug 6, 2021
Note the SDK is being updated separately to actually implement support for this property in dotnet/sdk#19521

Addresses #35131
DamianEdwards added a commit to dotnet/aspnetcore that referenced this issue Aug 7, 2021
Note the SDK is being updated separately to actually implement support for this property in dotnet/sdk#19521

Addresses #35131
@RussKie
Copy link
Member

RussKie commented Aug 9, 2021

To call out a dependency - Windows Desktop (i.e. Windows Forms and WPF) can't be updated until this change is implemented.

/cc: @ryalanms

@AraHaan
Copy link
Member

AraHaan commented Aug 10, 2021

nit: Suggest avoiding any parsing of the <Using /> item spec e.g.

<Using Include="Microsoft.AspNetCore.Http.Results" Alias="Results" />

and

<Using Include="Microsoft.AspNetCore.Results" Static="true" />

With this will it also be valid to also do stuff like:

<Using Include="System.Runtime.Loader" Condition="'$(TargetFramework)' == 'net5.0'" />

as well?

@alexrp
Copy link

alexrp commented Aug 10, 2021

Yes, conditions work on any kind of MSBuild item.

@dsplaisted
Copy link
Member

@DamianEdwards I do not think we should support enable as a value for the ImplicitUsings property. Other Boolean MSBuild properties just support true. The Nullable property uses enable instead of true because it has 4 different supported values instead of just true and false: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references#nullable-contexts

@DamianEdwards
Copy link
Member Author

DamianEdwards commented Aug 10, 2021

@dsplaisted from the PR

I put it in the spec for three reasons, one of them was to align with <Nullable>, another was to better match the naming (boolean properties typically start with a verb like Enable or Disable), the other was to leave space for expanding the supported values in the future, as was suggested during the review meeting last week, so the feature could support more than just a simple, on/off.

@AraHaan
Copy link
Member

AraHaan commented Aug 10, 2021

@DamianEdwards Any ETA on when all this can be completed and in the daily rc.1 builds so I can try it locally / on my repositories in CI?

Yes I rigged up my CI (github actions) to always install the daily quality builds from the .NET install scripts.

@DamianEdwards
Copy link
Member Author

@AraHaan hopefully within the next week.

@DamianEdwards
Copy link
Member Author

@wli3 RE the value enabled see discussion in PR here.

@wli3
Copy link

wli3 commented Aug 19, 2021

@DamianEdwards I think the users will have no way to figure that out themselves. I need to run a /pp:out.xml to realize the import order. And few people know the 3 pass of msbuild evaluation

@AraHaan
Copy link
Member

AraHaan commented Aug 19, 2021

I think the best way is to in the future move away from setting things in the csproj and instead put everything in an Directory.Build.props (that are not items in msbuild / properties that can properly be set there) and then an Directory.Build.targets file for everything else, while Directory.Packages.props contains all project or package references.

I already do that but I think updated templates for that might actually help out a lot more.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Aug 19, 2021
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>
@DamianEdwards
Copy link
Member Author

@wli3 this is already the case today for many aspects of the SDK that we default, including file globbing, versions, etc. The difference in evaluation order between properties and items is long-standing and while it's not particularly discoverable it's how MSBuild works.

@wli3
Copy link

wli3 commented Aug 19, 2021

The different here is, once we made it default someday, users have to migrate their code base over, and disabling an certain import across repo would be a common thing to do. Most of the "advance" SDK config are not that common.

@DamianEdwards
Copy link
Member Author

Doesn't change the fact that MSBuild works this way. And VB has had this feature for a very long time, on by default. If we do investigate making this feature on by default in a future SDK major release we'll need to look at potential issues associated with the change as part of that effort.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Aug 20, 2021
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>
@martincostello
Copy link
Member

martincostello commented Aug 23, 2021

A minor issue with this I've just come across trying out the new <Using> item with a daily RC1 build (6.0.100-rc.1.21420.39).

I have a library that targets the following TFMs to share some simple POCOs:

<TargetFrameworks>net461;netstandard2.0;netstandard2.1;net5.0;net6.0</TargetFrameworks>

The net461 library fails to compile with the following error:

Severity	Code	Description	Project	File	Line	Suppression State
Error	CS0234	The type or namespace name 'Http' does not exist in the namespace 'System.Net' (are you missing an assembly reference?)	MyApp.Models (net461)	C:\Coding\MyApp\src\MyApp.Models\obj\Debug\net461\MyApp.Models.GlobalUsings.g.cs	7	Active

I can work around this by adding the following to the .csproj file, but it would seem to make sense to maybe gate that particular using on the targeted TFM as it's not a "built-in" using for .NET Framework:

<ItemGroup>
  <Using Remove="System.Net.Http" />
</ItemGroup>

@KalleOlaviNiemitalo
Copy link

The error might be covered by the disclaimer that C# 10.0 is supported on .NET 6 only.

@DamianEdwards
Copy link
Member Author

Yes this scenario (multi-targeting) was discussed during discussions for this update and and as @KalleOlaviNiemitalo points out, given that C# 10 is not supported on anything other than .NET 6, projects changing <LangVersion> manually and setting <ImplicitUsings>enable</ImplicitUsings> are considered to be examples of an advanced/edge scenario and that we should simply honor what the user is configuring their project to do (i.e. opt out of default lang versions but opt-in to the generation of implied global usings).

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Aug 23, 2021
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 added a commit to jonathanpeppers/xamarin-android that referenced this issue Aug 23, 2021
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>
@jmaillet
Copy link

Why is this not just driven by the template? Have an _Usings.cs in the project by default, like Razor does. Much easier to see what namespaces included by default. No need to google the documentation. Personally, I'd much rather just stay in my c# headspace and edit that file to add or remove namespaces, as opposed to mucking with the csproj file with a completely different syntax.

@drewnoakes
Copy link
Member

@DamianEdwards please take a look at dotnet/project-system#7512 which adds a flag to enable/disable this feature from the Project Properties UI. In future we can add UI to control the specific set of items.

jonpryor pushed a commit to dotnet/android that referenced this issue 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
jonathanpeppers added a commit to dotnet/android that referenced this issue 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
@gewarren
Copy link

gewarren commented Sep 2, 2021

Are the SDK-specific MSBuild properties still valid as of RC1? For example, DisableImplicitNamespaceImports_DotNet, which may have been renamed to ImplicitUsings_DotNet?

Edit: I see from Mikayla's question that these have been removed.

@DamianEdwards
Copy link
Member Author

Closing as this is complete now.

@drewnoakes
Copy link
Member

A minor issue with this I've just come across trying out the new <Using> item...

I have a library that targets the following TFMs to share some simple POCOs:

<TargetFrameworks>net461;netstandard2.0;netstandard2.1;net5.0;net6.0</TargetFrameworks>

The net461 library fails to compile with the following error:

The type or namespace name 'Http' does not exist in the namespace 'System.Net' (are you missing an assembly reference?)

@martincostello while unsupported, you may be able to solve this problem using the approach described in dotnet/project-system#7488 (comment)

@kirkpabk
Copy link

kirkpabk commented Oct 14, 2022

Disable unnecessary usings statements. The ambiguous references seems to be a result of a compiler bug or feature which gets confused when implicit using and unnecessary usings statements are used.

SDK Default namespaces
Microsoft.NET.Sdk

  • System
  • System.Collections.Generic
  • System.IO
  • System.Linq
  • System.Net.Http
  • System.Threading
  • System.Threading.Tasks

Microsoft.NET.Sdk.Web

  • System.Net.Http.Json
  • Microsoft.AspNetCore.Builder
  • Microsoft.AspNetCore.Hosting
  • Microsoft.AspNetCore.Http
  • Microsoft.AspNetCore.Routing
  • Microsoft.Extensions.Configuration
  • Microsoft.Extensions.DependencyInjection
  • Microsoft.Extensions.Hosting
  • Microsoft.Extensions.Logging

Microsoft.NET.Sdk.Worker

  • Microsoft.Extensions.Configuration
  • Microsoft.Extensions.DependencyInjection
  • Microsoft.Extensions.Hosting
  • Microsoft.Extensions.Logging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests