dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.21k stars 9.95k forks source link

Enable IDE0005 analyzer in the build #47912

Open eerhardt opened 1 year ago

eerhardt commented 1 year ago

See the conversation in https://github.com/dotnet/aspnetcore/pull/47540#issuecomment-1523799428. In order to unblock updating the SDK, I needed to add dotnet_diagnostic.EnableGenerateDocumentationFile.severity = none to the .globalconfig file. The new SDK was breaking the build for projects that don't have GenerateDocumentationFile=true.

We should figure out how to enable GenerateDocumentationFile=true for non product assemblies.

@sharwell suggested adding:

  <PropertyGroup>
    <!--
      Make sure any documentation comments which are included in code get checked for syntax during the build, but do
      not report warnings for missing comments.
      CS1573: Parameter 'parameter' has no matching param tag in the XML comment for 'parameter' (but other parameters do)
      CS1591: Missing XML comment for publicly visible type or member 'Type_or_Member'
      CS1712: Type parameter 'type_parameter' has no matching typeparam tag in the XML comment on 'type_or_member' (but other type parameters do)
    -->
    <GenerateDocumentationFile>True</GenerateDocumentationFile>
    <NoWarn>$(NoWarn),1573,1591,1712</NoWarn>
  </PropertyGroup>

I think we should only set those NoWarn for non-product level assemblies. So we need to do some build logic to ensure those doc warnings still occur for product projects that have public API.

More info at:

mkArtakMSFT commented 1 year ago

@eerhardt given that the SDK update was unblocked already, how urgent is this? Also, @wtgodbe mentioned that the next SDK update may enable this analyzer by default. Is there more for us to do here then? /cc @captainsafia

eerhardt commented 1 year ago

how urgent is this?

Not urgent. Just know that we aren't getting the IDE0005 diagnostic working in all projects because of this.

Is there more for us to do here then?

Yes. The work is as I describe above.

ryanbuening commented 1 year ago

I just updated my Visual Studio to 17.7 GA and am now seeing this issue in my .NET 7 project when building.

image

Removing dotnet_diagnostic.IDE0005.severity = error from the .editorconfig fixes it.

madhusameena commented 1 year ago

I just updated my Visual Studio to 17.7 GA and am now seeing this issue in my .NET 7 project when building.

image

Removing dotnet_diagnostic.IDE0005.severity = error from the .editorconfig fixes it.

Same with me, any workaround for this? I cant disable IDE0005 completely

scottkuhl commented 1 year ago

I'm seeing the same thing after updating Visual Studio for Mac as well.

sharwell commented 1 year ago

Also, @wtgodbe mentioned that the next SDK update may enable this analyzer by default.

What do you mean "this analyzer"? It would be unfortunate to enable IDE0005 at warning level by default. In team use, it has been observed to increase the frequency of merge conflicts. Eventually we just turned it off in Roslyn and there appears to be no downside to leaving it that way.

sharwell commented 1 year ago

Same with me, any workaround for this? I cant disable IDE0005 completely

The compiler requires one of the following two states:

  1. IDE0005 is set to suggestion (or lower) severity
  2. GenerateDocumentationFile is set to true

Note that it is trivial to meet the second requirement by using the XML <PropertyGroup> syntax shown in the first post. With those three specific documentation warnings disabled, GenerateDocumentationFile will only produce new warnings in the build if you have invalid XML comments already contained in source code.

If you attempt to "work around" this issue by trying to disable the EnableGenerateDocumentationFile, then it will not be successful because that approach is effectively the same as option (1) - it causes IDE0005 to be silently disabled.

I cant disable IDE0005 completely

If EnableGenerateDocumentationFile is showing up in your build, then IDE0005 is already disabled, even though the project asked for it to be enabled. The warning is alerting developers to the fact that the compiler cannot provide the functionality that the project has explicitly requested because the project is incorrectly configured.

scottkuhl commented 1 year ago

If you set GenerateDocumentationFile to true, that will bring back this issue, Warning when referencing a project with GenerateDocumentationFile true, on MAUI projects.

sharwell commented 1 year ago

That issue was marked as fixed. If you are still using an older version of the SDK, it should be possible to update the XML file item in the build to include the expected metadata, though I'm not exactly sure where that update would need to be placed (it would have to be after the location where the SDK converts GenerateDocumentationFile to the actual build item that gets copied). The other option would be if there is some property that allows the file to be generated by the compiler, but not copied to the output.

scottkuhl commented 1 year ago

It may have been marked as fixed, but I’m on the latest version of 7 and it still exists.

LiviuD commented 1 year ago

Still present in 17.7.2

VictorioBerra commented 1 year ago

Setting EnableGenerateDocumentationFile to true for me lit up all sorts of errors for the .editorconfig I am using: https://github.com/Dotnet-Boxed/Templates/blob/main/.editorconfig

sharwell commented 1 year ago

@VictorioBerra If those warnings were CS1573, CS1591, and/or CS1712, the solution is given in the very first post of this thread. If it was other warnings starting with CS, it means the project contained documentation comments containing some sort of syntax error which is almost certainly not intended. If it was another error, I would need more information to better understand.

LiviuD commented 1 year ago

That's how Microsoft sees the issue: https://developercommunity.visualstudio.com/t/EnableGenerateDocumentationFile-Error/10448023?viewtype=solutions This cannot be "by design" when this happens all of a sudden once you update to 17.7 version and while before all was nice and working fine.

EGUSAXO commented 1 year ago

It is very annoying - all our projects started showing this error after updating .net sdk. I would like to have IDE0005 enabled, but I don't want to enable "GenerateDocumentationFile", as it starts generating errors in old code, plus I think generating this file will make builds slower.

I don't see how IDE0005 and "GenerateDocumentationFile" are related, it should be perfectly fine to enable just one and not other.

patricksadowski commented 1 year ago

It is very annoying - all our projects started showing this error after updating .net sdk. I would like to have IDE0005 enabled, but I don't want to enable "GenerateDocumentationFile", as it starts generating errors in old code, plus I think generating this file will make builds slower.

I don't see how IDE0005 and "GenerateDocumentationFile" are related, it should be perfectly fine to enable just one and not other.

To keep the previous behavior we use <NoWarn>$(NoWarn);EnableGenerateDocumentationFile</NoWarn>.

LiviuD commented 1 year ago

It is very annoying - all our projects started showing this error after updating .net sdk. I would like to have IDE0005 enabled, but I don't want to enable "GenerateDocumentationFile", as it starts generating errors in old code, plus I think generating this file will make builds slower. I don't see how IDE0005 and "GenerateDocumentationFile" are related, it should be perfectly fine to enable just one and not other.

To keep the previous behavior we use <NoWarn>$(NoWarn);EnableGenerateDocumentationFile</NoWarn>.

Hi Patrick, Where do you put this: $(NoWarn);EnableGenerateDocumentationFile?

Thank you!

sharwell commented 1 year ago

Hi @LiviuD,

I would strongly discourage using $(NoWarn);EnableGenerateDocumentationFile. This will not fix the problem, but will instead directly instruct the build to produce an incorrect output. If you do not want to enable documentation output, the only currently-correct solution is to reduce the severity of IDE0005 to suggestion. If customers keep trying to work around this issue by disabling EnableGenerateDocumentationFile, we will likely be forced to rename the diagnostic or add a new diagnostic to force it to appear again.

I would like to have IDE0005 enabled, but I don't want to enable "GenerateDocumentationFile"

This is not possible today, but there is an open feature request to make it possible: https://github.com/dotnet/roslyn/issues/26938

I don't see how IDE0005 and "GenerateDocumentationFile" are related, it should be perfectly fine to enable just one and not other.

Note that this is literally the reason why the EnableGenerateDocumentationFile warning exists. The build is silently producing a constraint that almost no customers were aware of.

LiviuD commented 1 year ago

Thank you very much @sharwell for your suggestion. Can you please point me in the right direction on how can I reduce the severity of IDE0005 to suggestion, for an entire solution or for the entire machine?

Thank you!

glen-84 commented 1 year ago

@LiviuD,

Add this to a .editorconfig file:

[*.cs]
dotnet_diagnostic.IDE0005.severity = suggestion

The suggestions won't show up during a build/dotnet-format though. You can use dotnet format --verify-no-changes --severity info, but that may show additional suggestions.

sharwell commented 1 year ago

@LiviuD The default severity of IDE0005 is either suggestion or hidden. The correction would need to be made in the location where IDE0005 was customized in your build to have a non-default value. git grep IDE0005 may help locate the place where it was customized.

LiviuD commented 1 year ago

Hi @sharwell, Thank you for your suggestion, this seems to do the trick. I was able to find the customisation in a Warnings.props file, where IDE0005 warning was set to appear as an error. Maybe this will help others, so it is not only in .editorconfigs files that you need to look/change.

ghost commented 9 months ago

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.