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

Add global usings for .NET projects (.NET SDK, Web and Worker) #18459

Merged
merged 44 commits into from
Jul 8, 2021

Conversation

JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Jun 23, 2021

Fixes dotnet/aspnetcore#32451.

TODOs:

  • Verify that preview condition should be removed
    • Ensure SDK is updated to support C# 10
  • Add test
  • Review additional usings:
    • System.ComponentModel.DataAnnotations
    • Microsoft.Extensions.Caching.Memory
    • Microsoft.AspNetCore.Mvc\
  • Checkin with VB expert
  • Perf numbers using sample minimal API Empty Web project: https://github.com/JunTaoLuo/GlobalUsings/tree/main/App
Mean Median Std. Dev.
Before (usings in Program.cs) 2253.425ms 2266.458ms 107.8083ms
After (usings in generated App.ImplicitNamespaceImports.cs): 2221.974ms 2214.453ms 104.5122ms

Followups:

  • Underscore in naming: e.g. DisableImplicitNamespaceImports_Web
  • Inclusion of System.Net.Http in the base SDK
  • Relocation of VB Imports to props files
  • Breaking change announcements
    • dotnet/Docs
    • blog post

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@davidfowl
Copy link
Member

2 more considerations for global usings:

  • System.ComponentModel.DataAnnotations
  • Microsoft.Extensions.Caching.Memory

@JunTaoLuo
Copy link
Contributor Author

JunTaoLuo commented Jun 24, 2021

System.ComponentModel.DataAnnotations
Microsoft.Extensions.Caching.Memory

Hmm, what are the common use cases for these two? I'd assume MVC stuff for S.CM.DA but for M.E.C.M is it really used that frequently? I feel I rarely create one, but sometimes use other things that internally rely on it.

@JunTaoLuo
Copy link
Contributor Author

Ah I found some justification for the two new usings in offline conversations, colour me somewhat convinced.

DamianEdwards added a commit to dotnet/aspnetcore that referenced this pull request Jul 10, 2021
-Depends on dotnet/sdk#18459 flowing to dotnet/aspnetcore repo before we can merge this
- There's a couple of places we could remove usings from .razor files but need to verify the global usings flow into Razor compiler (#34217)
- Reordered usings in some places to ensure they get emitted in alphabetical order (modulo System.* & Microsoft.* coming before any others)
RussKie added a commit to dotnet/wpf that referenced this pull request Jul 15, 2021
RussKie added a commit to dotnet/wpf that referenced this pull request Jul 19, 2021
@RussKie
Copy link
Member

RussKie commented Jul 26, 2021

To be honest I'm finding the global usings feature very intrusive. I'm on the 3rd dotnet/* repo that updating to 6.0.100-rc.1.*, and I have to deal with error CS8652: The feature 'global using directive' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. errors.
Even though I participated in the disucssion of the original implementation I still have to go and search GitHub for the property I need to set to false because I don't want to update thousands of source files removing usings. I think an average developer wouldn't know what to do, and will just get massively pissed off.

❗ Btw please note the typo in lang uage version text. [edit] I actually think it is the wrapping on the cli. All good.

@davidfowl
Copy link
Member

A couple of questions:

  • What types of projects are these?
  • Are you using VS 2019?

@RussKie
Copy link
Member

RussKie commented Jul 26, 2021

dotnet/winforms, dotnet/winforms-designer, dotnet/wpf
Dev17 latest

@davidfowl
Copy link
Member

I don't think those repositories are representative of what the average developer will experience. Have you used the global usings in a real project that doesn't use arcade based repository? Also:

  • VS 2019 doesn't fully support C# 10
  • Are these projects .NET 6 or .NET standard?

@RussKie
Copy link
Member

RussKie commented Jul 26, 2021

The point I'm trying to make - if a developer tries to migrate an existing project to .NET 6.0 right now the developer will be likely be "greeted" with the error above.
I understand the desire to use global usings in new projects, but for existing established projects having it on by default IMO is an unnecessary churn with no real value add.

@davidfowl
Copy link
Member

The point I'm trying to make - if a developer tries to migrate an existing project to .NET 6.0 right now the developer will be likely be "greeted" with the error above.

Right, specifically because they will be trying to use .NET 6 with VS 2019 where C# 10 support is dodgy.

I understand the desire to use global usings in new projects, but for existing established projects having it on by default IMO is an unnecessary churn with no real value add.

That's why the option exists to turn it off.

@dotMorten
Copy link

dotMorten commented Jul 28, 2021

I'm a bit confused here. The issue this PR references only discusses sdk.web and sdk.worker, but it seems the basic .NET SDK is also addressed in this PR?

In general this feature IMHO causes more issues than it solves. The errors it can introduce in your build are very unhelpful to discover what the root cause is. There is no where in your code or project settings where those global namespaces are defined, since it is all implicit. I'm already seeing a slew of PRs in the .net repos opting out of this behavior, which to me indicates the problem with this.

I can also see that WinForms and WPF are planning to add more implicit namespaces via their SDKs, Xamarin iOS and Android already did it, and I wouldn't be surprised if more nuget packages will start doing the same thing. This means that as you're adding dependencies, your code could immediately break, and it will quickly get completely out of hand. (oh and currently - temporarily - setting UseWpf to true will actually disable the feature, so again referencing a dependency might break you the other way around as well). The issue actually points out that this can go wrong if it's done too broadly which is exactly what is happening with all the SDKs starting to pile on to it:

We of course do need to be careful not to overdo this and introduce a bunch of conflicts or make intellisense too confusing.

While I do think the idea of global namespaces is great, I'm rather concerned about implicitly defining them. I'd rather this was done at a per-project level instead, either via an Imports.cs file, or a project setting that explicitly opted-in instead, and the project templates took advantage of it. I get the goal here is to make your first hello-world project easier to get started with, but I still think that's possible with your first "dotnet new [...]" if it is in the template. I am however worried about the effect this has on every existing project out there, and it affects every single person who is already over their initial hello-world stage.

@lukemcdo
Copy link

I'd like to point out that the issues aren't just for old code, they're for new code too. Adding a global using to a project template might already be breaking, but removing one from a project template is now a massive consideration that will break third party docs that reference the template.

And if you can't use them in templates for above reason then what's the point? You're still showing new users all of the namespaces anyway then, realistically.

@davidfowl
Copy link
Member

I'm not convinced this is a bad idea yet, I think we have to make a few tweaks to deal with the set of issues we've seen so far but I haven't seen enough to make me want to pull the plug:

  • Our repositories aren't a good representation of what users will run into.
  • Most people are still on VS 2019 which doesn't fully support C# 10.

Some common problems that I’ve seen in the wild so far:

  • Multi-targeted projects end up complaining about the C# compiler version.
  • Projects that extend MSBuild get conflicts between MSBuild Task and System.Threading .Task
  • VS2019 complains about conflicts because it supports a subset of C# 10
  • Explicitly specifying the C# version to a version < 10 results in errors and you need to disable the global usings when this happens.

Adding a global using to a project template might already be breaking, but removing one from a project template is now a massive consideration that will break third party docs that reference the template.

Yes that's a known issue. This is why we're being conservative with what we add to them. They version nicely with TFMs so an explicit upgrade is required to see new implicit usings.

I think there are a few tactical things we can do here:

  • Disable implicit usings when in VS earlier than 2022 (not sure we can do anything for omnisharp). Or maybe we can detect it some other way
  • Disable implicit usings when the language version is explicitly set to < 10

@lukemcdo
Copy link

lukemcdo commented Jul 28, 2021

Fair enough, but like, I still don't understand the benefit. Really feels like the goal here is to smooth the very beginning first lines of C#, but like, this doesn't make the mid-level of learning C# (and to be clear this seems very specific to C#) any more straightforward.

I also don't like the way that it creates side effects across files. There was a point where the mono project was trying to do single-file projects for example, with the idea being you could just import via copy/paste an entire library. Some projects still aim for this.

Could we get something like "using Globals" to opt into using globals, or something stupid like it? I don't know, anything is better than this.

@dotMorten
Copy link

dotMorten commented Jul 28, 2021

Most people are still on VS 2019 which doesn't fully support C# 10.
[...]
Disable implicit usings when the language version is explicitly set to < 10

That's merely a temporary problem/solution.

Disable implicit usings when in VS earlier than 2022

I'd discourage that. It's nice you can use newer and older VS (depending on user) and still work and compile the same way.

we're being conservative with what we add to them

Browsing the recent commits in the various .NET repos, 'conservative' isn't what I would call adding more and more globals with each dependency (xamarin, wpf, winforms etc). And with it currently just being an item in an itemgroup, we'll start seeing nuget packages starting to abuse this as well.

@davidfowl
Copy link
Member

That's merely a temporary problem/solution.

Based on what I've seen that's the bulk of the problem that I've seen so far. With a smattering of ambiguous types (MSBuild Task vs Threading Task)

I'd discourage that. It's nice you can use newer and older VS (depending on user) and still work and compile the same way.

Maybe, "compile the same way" is impossible when the older version of VS doesn't support the language features.

Browsing the recent commits in the various .NET repos, 'conservative' isn't what I would call adding more and more globals with each dependency (xamarin, wpf, winforms etc). And with it currently just being an item in an itemgroup, we'll start seeing nuget packages starting to abuse this as well.

We're still in "wait and see" mode.

@RussKie
Copy link
Member

RussKie commented Jul 30, 2021

I'm on Dev 17.0.0 Preview 3.0 [31523.460.main], have 6.0.100-rc.1.21375.6 installed globally, trying to build a .NET 6.0 project targeting C# 9.0.

Why am I getting this error?
image

@davidfowl
Copy link
Member

Because you downgraded the language version to 9 and we currently don't gate the feature on that (though it's being discussed).

@swythan
Copy link

swythan commented Aug 2, 2021

We're still in "wait and see" mode.

@davidfowl Where is the best place to provide feedback on this? Commenting/voting on a closed PR seems likely to be ineffective.

@davidfowl
Copy link
Member

Here is a good place for now.

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

Successfully merging this pull request may close these issues.

Add default global usings to Microsoft.NET.Sdk.Web