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

Additional C# global imports #18825

Closed
JunTaoLuo opened this issue Jul 9, 2021 · 39 comments
Closed

Additional C# global imports #18825

JunTaoLuo opened this issue Jul 9, 2021 · 39 comments

Comments

@JunTaoLuo
Copy link
Contributor

There were some additional discussions on what global imports should or should not be included in the base .NET SDK. Specifically, there were concerns about:

✅ = currently included
❌ = not currently included

  1. ✅ System.Net.Http: The contention is that this is used for web concerns so maybe it belongs in the Web SDK instead. However, it's also very frequently used in general, even console apps.
  2. ❌ System.Net: This is often used in combination with System.Net.Http. We should put this in where-ever we end up putting System.Net.Http
  3. ❌ System.Diagnostics: This has been used in some of the Web templates. Is this a common enough concern to be included in an SDK, be it the base .NET SDK or Web SDK?
  4. ❌ System.Text: This is another commonly used namespace that we can consider for the base .NET SDK. Historically, this has been used

In the interest of ensuring all discussion is happening in the same place, let's use this issue as much as possible to track discussions. @DamianEdwards @RussKie.

FYI @davidfowl @halter73 @KathleenDollard @pranavkm @terrajobst

Feel free to tag anyone else who would be interested in this dicsussion. I've only included folks who have had additional input on the current import list.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Jul 9, 2021
@DamianEdwards
Copy link
Member

Here's the PR that removes using statements from the web templates based on what's implicitly added by the SDK now. The namespaces that are left are a potential place to start WRT to what to consider for further inclusion.

@JunTaoLuo
Copy link
Contributor Author

Also, as a reference here's the currently included list:

  1. Base .NET SDK
    <Import Include="System" />
    <Import Include="System.Collections.Generic" />
    <Import Include="System.IO" />
    <Import Include="System.Linq" />
    <Import Include="System.Net.Http" />
    <Import Include="System.Threading" />
    <Import Include="System.Threading.Tasks" />
  1. Web SDK (also includes everying in the base .NET SDK)
    <Import Include="System.Net.Http.Json" />
    <Import Include="Microsoft.AspNetCore.Builder" />
    <Import Include="Microsoft.AspNetCore.Hosting" />
    <Import Include="Microsoft.AspNetCore.Http" />
    <Import Include="Microsoft.AspNetCore.Routing" />
    <Import Include="Microsoft.Extensions.Configuration" />
    <Import Include="Microsoft.Extensions.DependencyInjection" />
    <Import Include="Microsoft.Extensions.Hosting" />
    <Import Include="Microsoft.Extensions.Logging" />
  1. Worker SDK (also includes everying in the base .NET SDK)
    <Import Include="Microsoft.Extensions.Configuration" />
    <Import Include="Microsoft.Extensions.DependencyInjection" />
    <Import Include="Microsoft.Extensions.Hosting" />
    <Import Include="Microsoft.Extensions.Logging" />

@davidfowl
Copy link
Member

davidfowl commented Jul 10, 2021

Seems like we missed the Blazor SDK for default usings. This change removed usings from the blazorwasm template (Microsoft.Extensions.*) but we didn't do the default usings there. I think we should fix it in the sdk instead of putting the usings back and additionally add more default usings for blazor wasm.

This currently is blocking SDK merges into the installer dotnet/installer#11100

PS: I've sent a PR to add Microsoft.Extensions only. I'm sure we'll want to add more

cc @danroth27 @SteveSandersonMS @pranavkm @javiercn

@pranavkm
Copy link
Contributor

I think we should fix it in the sdk instead of putting the usings back and additionally add more default usings for blazor wasm.

That creates some inconsistently between Server and WASM. Would we feel ok adding things to the Server's csproj to make them more consistent?

@davidfowl
Copy link
Member

@pranavkm can you clarify? Blazor server uses the web sdk no? Which has these usings and more...

@pranavkm
Copy link
Contributor

Do you have a list in mind that would go in the WebAssembly SDK? I'd think most of these would be from the Microsoft.AspNetCore.Components.* namepace?

@davidfowl
Copy link
Member

Do you have a list in mind that would go in the WebAssembly SDK? I'd think most of these would be from the Microsoft.AspNetCore.Components.* namepace?

I think that's better determined by the Blazor team. Also it seems like most of those things come from the package so maybe it doesn't make sense in the sdk?

@marcpopMSFT marcpopMSFT added this to the Backlog milestone Jul 19, 2021
@marcpopMSFT marcpopMSFT removed the untriaged Request triage from a team member label Jul 19, 2021
@marcpopMSFT
Copy link
Member

For reference, a quick GH search revealed 3M, 6M, 9M, and 9M results for items 1, 2, 3, and 4 in the original descriptions, respectively. For reference, Generic, IO, and Linq have 56M, 14M, and 36M, respectively.

Example query: https://github.com/search?q=%22System.Collections.Generic%22

@DamianEdwards
Copy link
Member

@marcpopMSFT worth pointing out that Generic, IO and Linq have been in the default item templates for Class etc. for many releases now and that's very likely contributing to the hit count WRT code base searches.

@davidfowl
Copy link
Member

Unrelated grep.app is superior to GitHub search

@marcpopMSFT
Copy link
Member

@davidfowl grep.app only shows a fractional number of results though so won't tell us the actual savings of making these implicit. Definitely better if we're looking to drill in.

@DamianEdwards , isn't some of the goal here to be able to remove extra usings from our templates (even unused ones) to simplify the experience for new customers.

@DamianEdwards
Copy link
Member

@marcpopMSFT yes of course, but I thought we already added the usings you point out have the huge hit counts? Sorry, maybe I misunderstood the intent of your comment (i.e. implying that the suggested namespaces aren't as popular as the ones already added so don't deserve to be implicit).

@marcpopMSFT
Copy link
Member

@DamianEdwards Ahh, I was pointing out that we already covered the really big ones and the new ones we're considering are a much smaller benefit (not that 3M isn't still a valueable but there is a cutoff point where it's no longer worth doing, I'm just not sure where that point is).

@DamianEdwards
Copy link
Member

DamianEdwards commented Jul 20, 2021

@marcpopMSFT so my point still stands I guess? We'll inherently get huge hits counts on those namespaces because they've been added by our item templates for many releases and many folks don't remove unused usings. So it's perhaps not a fair comparison.

@JunTaoLuo JunTaoLuo removed their assignment Jul 30, 2021
@JunTaoLuo
Copy link
Contributor Author

@davidfowl @DamianEdwards @halter73 who should be tracking this discussion?

@davidfowl
Copy link
Member

davidfowl commented Jul 30, 2021

I think we should add Microsoft.Extensions.Primitives to ASP.NET Core applications because of StringValues for header access.

@halter73
Copy link
Member

Given StringValue's implicit conversions to and from string and string[] is the using really necessary for most apps accessing headers?

@davidfowl
Copy link
Member

I hit it when I tried to use StringValues.IsNullOrEmpty

@swythan
Copy link

swythan commented Aug 2, 2021

@davidfowl Said general feedback on this can go here.

I really don't like the idea of the SDK adding implicit namespaces using statements to all my files. Clearly none of my existing files can be affected positively by such a change (as they already work), but using statements can introduce ambiguity into the compilation which can cause it to fail. With nothing apparent in either the file or project to indicate what namespaces are actually in scope it could be really tricky to debug.

I don't really understand how the feature can get past the "every new compiler feature starts with -50 points" attitude that the C# team always used to have. Everyone will have to learn which namespaces are available by default (and which aren't) which seems counter to the apparent intention of making life easier.

@davidfowl
Copy link
Member

@swythan Have you used it as yet?

@swythan
Copy link

swythan commented Aug 2, 2021

I've not, I'll admit.

It's not that I dislike the global usings feature; it seems really useful as something I can add to my own projects. It's just that this seems really wrong as a default. The idea that other NuGet packages can opt me in to even more namespaces fills me with dread.

It just seems like a good way to break things during upgrade (or worse, when adding a NuGet reference), and the value seems minimal. As people mentioned in the Twitter thread there seem like better ways to achieve the same thing which wouldn't hide what's going on (e.g. having the project templates add an imports.cs file).

Alongside that is the fact that once it's shipped then removing implicit namespaces would be a breaking change. It seems like it went in very close to the cut-off date for RC, and I only found out about it at all because I happen to follow the right people on Twitter.

@davidfowl
Copy link
Member

I'm going to agree but we're also waiting for some real feedback from usage.

@mattleibow
Copy link
Member

Why did the ASP.NET, WPF and WinForms repos disable it?

@davidfowl
Copy link
Member

I can't speak for WPF and Winforms, but in ASP.NET it was disabled pre-emptively because we're building the product itself .e.g We can't auto import namespaces that we're building from everywhere because we're too low on the stack. That's one of the reasons we can't use them in dotnet runtime globally as well. We could have allowed auto importing just the System.* namespaces and skipped the web namespaces and called it a day.

Also I think the pattern we've seen so far be problematic is class libraries that multi target. That seems to be where most of the pain is coming from if you ignore the VS 2019 vs 2022 C# 10 support issue.

@RussKie
Copy link
Member

RussKie commented Aug 2, 2021

WPF and WinForms repos disable it?

We didn't meet the Preview7 cut off deadline, so our global usings are coming in RC1.
That said I applied global usings to dotnet/winforms repo and I had to update 1,300+ files. Not the best use of my time. For dotnet/wpf I disabled the feature outright. For the dotnet/winforms-designer I had to disable it because we don't use C#10.

So far this feature has only caused me some grief, and I'm too of an opinion for established projects this feature must be opt-in instead of opt-out.

@MichalStrehovsky
Copy link
Member

The implicit namespace import caused a build break in crossgen2 because it has a low level implementation of LINQ (that has better perf characteristics than LINQ for people who don't do things like Where().Where().Select().Min()).

It was very much not obvious what caused it, even for a bunch of seasoned .NET devs. dotnet/runtime#55855

@MichalStrehovsky
Copy link
Member

Please consider all the current (and future!) names of types in all of the discussed namespaces, especially for things that go into the base SDK. I don't particularly care about the special-purpose SDKs - just for the base SDK. We market .NET as "A developer platform for building all your apps.". Does System.Net.Http make sense in a game? An IoT app? The default-included namespaces better make sense there or they just limit the number of available identifiers.

The prosed namespaces have some very common names in them that are going to conflict with existing things in the ecosystem (and if the conflicts happens to be in a type name within a NuGet package, the break is going to cascade out of that). People will no longer be able to use the common names because .NET essentially "reserves" them (causing extra friction if someone wants to use the common word in their own namespace going forward).

We have namespaces exactly because of this - to avoid conflicts.

E.g. System.Diagnostics has a Process class. So does Chocolatey, or Microsoft Terminal, or azure pipelines agent. The name Process makes sense there.

System.IO has a File class. Well so does Microsoft IIS Administration and countless others.

Basically, going overboard with the number of default-included namespaces we're making it hard to use common words as class names, while lacking any context whatsoever (base SDK is included for any app any developer any platform).

Frankly I'm finding it hard to justify including anything besides System in the base SDK. Even System is problematic as Jared would tell you, but I'm much less sympathetic to developers who name their classes String or Int32 than to those having their own Directory, Activity, Monitor, Timeout, or Task (that one is already hurting us).

@dsplaisted
Copy link
Member

@RussKie Glancing at your winforms PR, it looks like most of the updates were removing using statements for the same namespaces that were included in implicit global usings. As I understand it, you should not have needed to make those changes. I think you were probably using an older version of the Roslyn compilers. With more recent versions, those usings would not have caused errors and you wouldn't have needed to make most of those changes.

@davidfowl
Copy link
Member

As I understand it, you should not have needed to make those changes. I think you were probably using an older version of the Roslyn compilers. With more recent versions, those usings would not have caused errors and you wouldn't have needed to make most of those changes.

Yep see #18459 (comment)

@DamianEdwards
Copy link
Member

We're planning to make some changes to the implicit usings feature in rc.1 based on the feedback we've received here and elsewhere. Check out #19521 for details.

@SimonDD7
Copy link

Those namespaces shouldn't be accessed if not asked for. IntelliSense currently propose all classes from those namespaces in empty class file. VS marks System.IO reference as to be removed. This is really confusing.

@davidfowl
Copy link
Member

VS marks System.IO reference as to be removed

Can you elaborate here?

@SimonDD7
Copy link

SimonDD7 commented Aug 26, 2021

VS marks System.IO reference as to be removed

Can you elaborate here?

System.IO (as an example) is globally accessible after update to net6-preview7. So all those using directives are marked as unnecessary. Sorry, I should've been more specific.

@davidfowl
Copy link
Member

Ah yes, that's by design when this feature is on.

@Kaelum
Copy link

Kaelum commented Sep 13, 2021

I went down the rabbit hole with this, following the recommendations presented to me in after compiling, and I wasted 3 days working on this that I will never get back.

The error CS8652: The feature 'global using directive' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. message is the worst recommendation that I've come across. This is the last thing that anyone who is upgrading to .NET 6.0 will ever want to do. Instead, there should be a recommendation to use DisableImplicitNamespaceImports instead (took me an hour of reading though all of these issues to find it).

The sole purpose of the global::using statements is to reduce the compile time by milliseconds? I'm sorry, but the side effects are not worth it. As someone else stated, this should be a user setting not a global compiler setting. It seems like we're repeating history's failings, as something similar was done back in the early 90s, and it was a complete nightmare. None of which made it into the 1998 C++ specification.

The potential for abuse should be more than enough to limit this to an opt in feature, if this is something that you feel this strongly about adding. We already run into issues where we either need to fully specify the object name, or add a using Name = System.Name at the top of the code, which significantly reduces the readability of code. While we use Visual Studio, many people do not, which brings me back to the problems of the early '90s.

This feature is very similar to the code editor feature that aligns variable names, but that feature doesn't break anything. It might be nice to have for some, but he vast majority will most likely not opt in to all the problems that it brings. Unfortunately, we are currently forced to opt out, but we don't even know that we can opt out. Bad design decision made there.

@davidfowl
Copy link
Member

I'm gonna close this issue as there were major updates made and it doesn't reflect the current state of things. Use this issue to give more feedback:

#19521

@Kaelum
Copy link

Kaelum commented Sep 13, 2021

#19521 is closed as well.

@davidfowl
Copy link
Member

It is but I would read it first then open another issue with more details if that isn't satisfying

@pranavkm
Copy link
Contributor

The potential for abuse should be more than enough to limit this to an opt in feature, if this is something that you feel this strongly about

Starting in rc1, it is a opt-in feature. The other issue goes over some of the implementation details about this.

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