DotNetAnalyzers / StyleCopAnalyzers

An implementation of StyleCop rules using the .NET Compiler Platform
MIT License
2.66k stars 507 forks source link

SA1200 is firing for global usings #3404

Open Trolldemorted opened 3 years ago

Trolldemorted commented 3 years ago

As mentioned in https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3243, SA1200 is firing for C# 10 global using statements.

cc @AraHaan @sharwell

sharwell commented 3 years ago

Note that global using directives are only expected to appear in auto-generated files, since they are created by the SDK according to directives listed in the project file. When this pattern is followed, SA1200 will not be reported because SA1200 is not active in generated files.

Trolldemorted commented 3 years ago

Note that global using directives are only expected to appear in auto-generated files

Why would you expect that? It is totally valid to use them in normal code, to reduce the boilerplate across your code base.

sharwell commented 3 years ago

Consistent declaration of global usings in the project file reduces the burden of keeping track of which directives are declared in which location(s) throughout the project. This best practice is already supported by SA1200.

AraHaan commented 3 years ago

Some people use older .NET SDKs but reference a version of the compiler that supports global usings via nuget packages just so they can use newer C# language versions I think with it (when they set their C# language versions to preview or latest) while targeting an older framework. With that aproach they can then use global usings or file scoped namespaces but since they are using an older .NET SDK (where the SDK does not auto generate them) they have to write the global usings themselves.

Now I know you are thinking: Why are they wanting to do that?

The answer is that sometimes their CI pipelines does not allow them to update the .NET SDK they want to use and so they get stuck with an older one installed with no way to install an newer one without admin/root access (some lock that down to avoid them from installing malicious programs in their CI). Sometimes however they have to reference the C# compiler package due to the one in their SDK bugging on their code and the referenced one fixes it due to it being newer. Both cases are valid reasons why the analyzer should acknowledge global usings and not expect them to only happen on autogenerated files mainly because it is not always possible. Likewise perhaps the user prefers to type the global usings themselves?

sharwell commented 3 years ago

@AraHaan You can still add the following line to the top of a file manually if it's not possible to use the project system to generate the file:

// <auto-generated />
gyssels commented 2 years ago

Actually it is the new language feature. We can see being used manually on the event of the launch of VS2012.

Trolldemorted commented 2 years ago

@AraHaan You can still add the following line to the top of a file manually if it's not possible to use the project system to generate the file:

This will disable more warnings though, and not just the one this bug causes

fgimian commented 2 years ago

May I please ask that this be reconsidered? Global using statements are intended to be used in normal source code to reduce boilerplate in cases where certain libraries are bound to be imported in all / most files. It's clear that Microsoft has been working hard to reduce boilerplate with file-scoped namespaces, implied Main method, record types and global namespaces. This allows the language to be more succinct (sometimes to its detriment) but in many cases this is justified to reduce repeated code.

Would you at least consider an option to avoid firing SA1200 when global usings are used in regular source code please?

sharwell commented 2 years ago

Global using statements are intended to be used in normal source code to reduce boilerplate in cases where certain libraries are bound to be imported in all / most files.

The current behavior does not impede the use of global using directives via the project system support which is used by the SDK. The file containing the resulting generated using directives is marked with /// <auto-generated/> and automatically excluded from style analysis. This strategy completely removes the global using directives from user code.

Would you at least consider an option to avoid firing SA1200 when global usings are used in regular source code please?

It would be fine to update all the using directive rules to account for global using directives, but we would expect the change to include complete tests for all supported options/scenarios related to using directives. This is one of the more complicated analyzer/fix combinations, so I'm not sure when this will be completed considering the SDK approach for this feature naturally avoids problems.

fgimian commented 2 years ago

Global using statements are intended to be used in normal source code to reduce boilerplate in cases where certain libraries are bound to be imported in all / most files.

The current behavior does not impede the use of global using directives via the project system support which is used by the SDK. The file containing the resulting generated using directives is marked with /// <auto-generated/> and automatically excluded from style analysis. This strategy completely removes the global using directives from user code.

Would you at least consider an option to avoid firing SA1200 when global usings are used in regular source code please?

It would be fine to update all the using directive rules to account for global using directives, but we would expect the change to include complete tests for all supported options/scenarios related to using directives. This is one of the more complicated analyzer/fix combinations, so I'm not sure when this will be completed considering the SDK approach for this feature naturally avoids problems.

Thanks so much for the reply. For the moment I'm simply getting around it by disabling SA1200 within my GlobalUsings.cs file which works well enough. 😄

Keep up the great work Fotis