Closed JunTaoLuo closed 3 years ago
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.
Also, as a reference here's the currently included list:
<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" />
<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" />
<Import Include="Microsoft.Extensions.Configuration" />
<Import Include="Microsoft.Extensions.DependencyInjection" />
<Import Include="Microsoft.Extensions.Hosting" />
<Import Include="Microsoft.Extensions.Logging" />
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 https://github.com/dotnet/installer/pull/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
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?
@pranavkm can you clarify? Blazor server uses the web sdk no? Which has these usings and more...
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?
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?
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
@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.
Unrelated grep.app is superior to GitHub search
@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.
@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).
@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).
@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.
@davidfowl @DamianEdwards @halter73 who should be tracking this discussion?
I think we should add Microsoft.Extensions.Primitives
to ASP.NET Core applications because of StringValues for header access.
Given StringValue
's implicit conversions to and from string
and string[]
is the using really necessary for most apps accessing headers?
I hit it when I tried to use StringValues.IsNullOrEmpty
@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.
@swythan Have you used it as yet?
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.
I'm going to agree but we're also waiting for some real feedback from usage.
Why did the ASP.NET, WPF and WinForms repos disable it?
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.
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.
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. https://github.com/dotnet/runtime/issues/55855
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).
@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.
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 https://github.com/dotnet/sdk/pull/18459#issuecomment-888606605
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.
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.
VS marks System.IO reference as to be removed
Can you elaborate here?
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.
Ah yes, that's by design when this feature is on.
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.
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:
It is but I would read it first then open another issue with more details if that isn't satisfying
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.
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
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.