lombiq / .net-analyzers Goto Github PK

There have been quite a few releases of the packages we use, so update all of them to the latest released versions. Fix the new analyzer violations in OSOCE.

Jira issue

This currently only shows false alarms in our code bases. See: dotnet/roslyn#48876

Microsoft.CodeAnalysis.BannedApiAnalyzers, see docs here, can be used to ban selected classed, methods, properties, or else.

• Some inspiration for where to begin the default config. Gather your thoughts and that of our colleagues on what should be banned.
• Since the NuGet package doesn't have any docs, in our docs link to the help file linked above.

Jira issue

This is our follow-up issue once dotnet/roslyn#55526 is fixed.

What's the problem?

In Git repositories, it's common to use LF (\n, "Unix-style") line endings for better cross-platform compatibility (see Lombiq/Open-Source-Orchard-Core-Extensions#65). The problem is that with .cs files, if we have LF line endings instead of CRLF (\r\n, "Windows-style") then roslyn will emit several false-positive IDE0055 warnings. There is a workaround of making only the .cs files CRLF as described in the above-linked issue. We have applied it to this repo and its submodules during OSOE-49.

What needs to be done?

Once the analyzer is fixed, the *.cs text eol=crlf rule should be removed from all .gitattributes files and then all .cs files should be renormalized to LF using the same steps described in the issue description of OSOE-91.

Jira issue

Enable nullable reference types centrally and migrate OSOCE.

• See migration docs.
• Keep https://github.com/Lombiq/.NET-Analyzers#adding-a-new-analyzer in mind.
• Search projects for <Nullable> configs and remove them.

Jira issue

See e.g. this most recent example: Lombiq/Helpful-Libraries#261. I also started an investigation there.

Jira issue

Configure S103 to use our conventional line length config.

See: https://github.com/Lombiq/.NET-Analyzers#upgrading-to-a-new-version-of-the-net-sdk

• Update all packages to latest.
• Use <Features>strict</Features> see this post.
• Update AnalysisLevel.
• Update LangVersion.

Note that this also requires all projects using .NET Analyzers to be upgraded as well.

There's a task/net-6-upgrade branch already (from here: #36); start from that and merge the new PR to dev.

This has happened a couple of times, but very rarely. Restarting the workflow fixes the problem.
An example is here, however it's not related to that issue (compilation failed in Lombiq.SetupExtensions.csproj which doesn't use nodejs) and I've seen it appear in other branches too, it's just the latest one at the time of writing. I've checked the attached binlog but didn't see anything significant in it.

Jira issue

Check out Meziantou.Analyzer and see if it's something we should use. See the duplicate rules with NetAnalyzers here.

This needs to be tested on multiple bigger solutions.

...in Build.props. This shouldn't be needed (since it make the app be built with the compiler in the package, not in the SDK) but is necessary to work around dotnet/sdk#23972.

This is only suitable once .editorconfig has everything ruleset files provide already:

• IDE support, i.e. you don't have to manually add every rule but can just select what to do for each rule. This is now done with VS 2022. However, it only works for files under the solution, so you can't edit under the tools folder.
• Ability to extend another .editorconfig file. This is only interesting for overriding our default rules though (as we explain here). It would also help to get rid of the editorconfig copy to the solution root. However, perhaps the <EditorConfigFiles> could help us, see this blog post. An alternative would be dotnet/roslyn#19028 (comment).
• There's also .globalconfig, but that's only for analyzer configs, not edit configuration like charset or guidelines. See dotnet/roslyn#47707.

Tasks:

• Migrate the config files, see the official docs.
• Perhaps we need to have use case-specific packages, e.g. `Lombiq.Analyzers.OrchardCore"?
• Update docs. Search for ".editorconfig" and "ruleset".
• Add docs on migration, including converting ruleset files.
• Check if we need <developmentDependency>true</developmentDependency> in the nuspec template. The analyzer packages we use do this. Check the generated <PackageReference> after a new install and change the docs, including release notes, and consuming projects if necessary.
• Publish new NuGet package. This will need to be a major release. Add migration docs to the release.

See https://github.com/Lombiq/.NET-Analyzers#upgrading-to-a-new-version-of-the-net-sdk

Jira issue

Been using Lombiq.Analyzers as a submodule for a while but recently every analysis error is being thrown including ones disabled in the rules file like as if they don't exist.

If I disable below it makes no difference.

<Project>
  <ItemGroup> 
      <Analyzer Remove="@(Analyzer)" /> 
  </ItemGroup>
</Project>

Or via the command line using /p:RunCodeAnalysis=false - no difference.

It still is fine in our build pipeline however if I bring down the source code into a new computer - same issue.

Typical error example are:
error CA1707: Remove the underscores from member name Kast.ConnectCard.Constants.FeatureIds.Kast_ConnectCard_OnUs

I copied Lombiq's open source project and copied the configuration from there but this made no difference.

Let's develop custom analyzers. This is not that easy, and we'd need to publish them on NuGet since analyzers can't be used directly from the source.

Some ideas:

• Check if controller actions have authorization either with attributes (e.g. [Admin]) or with an explicit IAuthorizationService call.
• Check for any unfiltered ISession queries. While not always, usually there should be some kind of filtering instead of a full table retrieval.
• Check for IContentManager.GetAsync(string id) calls in a loop and suggest IContentManager.GetAsync(IEnumerable<string> contentItemIds, bool latest = false) instead.
• Check for OrchardCore.ResourceManagenet project/package reference if resource management tag helpers are used. Similarly, check for a missing OrchardCore.DisplayManagement reference and others. See OrchardCMS/OrchardCore#6943.
• Use IClock instead of DateTime.UtcNow.
• Interesting not just for Orchard apps but any ASP.NET Core app, so should be under their own category:
  • Check for the injected localizer to have a proper type (not the field/property it's saved to): It must be IStringLocalizer<MyClass> or IHtmlLocalizer<MyClass>. The type parameter mustn't be omitted or have a different type than the current type. This would actually be generally interesting for ASP.NET Core MVC apps. Came up here too: OrchardCMS/OrchardCore#10987
  • Check for the Sdk attribute being Microsoft.NET.Sdk.Razor in the csproj's <Project> element if Razor templates are being used.
  • Private and protected fields should be named _prefixedWithUnderscoreCamelCased except for T and H. We can modify SX1309 for this. The alternative would be to put all localizers into properties instead and enable SX1309.

Jira issue

The instructions cover adding this to your solution using a submodule method but do not cover how to utilise this from the NuGet package.

Could we have some clarification on this.

Currently, the rules in .editorconfig are not checked during build, only in the IDE. Running these during build will be available in the .NET 5 SDK (even if the app doesn't target .NET 5). We should then enable this.

See SonarSource/sonar-dotnet#3727.

See here. This is basically approval testing for public APIs.

Only useful for projects with an important published interface, like Helpful Libraries, especially libraries. So maybe we shouldn't add the analyzer package, just some common config and then you may add the analyzer to your own project only. However, the only available configuration is a single line.

Jira issue