We should enable CS1591 on projects that have migrated their docs

[ericstj]

@carlossanlop @safern

Once a project migrates its docs it should be a build error if docs are missing from a publicly visible member. I just tried this and it's not flagging CS1591.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @safern, @ViktorHofer, @Anipik
See info in area-owners.md if you want to be subscribed.

Issue Details

---

@carlossanlop @safern

Once a project migrates its docs it should be a build error if docs are missing from a publicly visible member. I just tried this and it's not flagging CS1591.

Author:	ericstj
Assignees:	-
Labels:	area-Infrastructure-libraries, untriaged
Milestone:	-

[safern]

Good catch. We will take care of this ASAP.

[carlossanlop]

@ericstj This could be addressed by manually adding:

<GenerateDocumentationFile>true</GenerateDocumentationFile>

to the first PropertyGroup in the *.csproj. Correct? Example

The tool does not add it yet because I didn't find a way to edit the csproj via the Roslyn/MSBuild APIs. But I think I can just do it using System.Text.Xml.

[carlossanlop]

I just saw your comment in your PR: https://github.com/dotnet/runtime/pull/49118/files#r587730539

So GenerateDocumentationFile is not enough, that's what you discovered?

[safern]

I believe you can use MSBuild ProjectCollection to load a project and modify it but not sure: https://docs.microsoft.com/en-us/dotnet/api/microsoft.build.evaluation.projectcollection.loadproject?view=msbuild-16-netcore#Microsoft_Build_Evaluation_ProjectCollection_LoadProject_System_String_

@ericstj should know, I believe he is done this before.

[ericstj]

GenerateDocumentationFile is different. As far as I can tell the only thing this impacts is the defintion of the DocumentationFile property:
https://github.com/dotnet/sdk/blob/d6232ee12e6cb1b9f0c6fabdcb961f943e9d7c6e/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets#L243-L256

Which may be irrelevant for our projects, since we define this ourselves:

runtime/src/libraries/Directory.Build.targets

Line 24 in 664e232

<DocumentationFile Condition="'$(IsSourceProject)' == 'true' and '$(DocumentationFile)' == '' and '$(MSBuildProjectExtension)' != '.vbproj'">$(OutputPath)$(MSBuildProjectName).xml</DocumentationFile>

I didn't need to define this property, I did it because the docs said I should. I found that CSC was already generating XML before I added it.

I'm not sure what we need to do to enable that code analysis rule. It needs to be investigated. Presumably whatever you find could be triggered off this property (or something similar) so you could only apply it to projects which were migrated.

To add a property to an MSBuild project you can use either the Evaluation or Construction APIs. Evaluation are easier to use but give you less control over where the property is written (since it's not a DOM, it's an OM)

[ViktorHofer]

This compiler warning is disabled in Arcade :(

https://github.com/dotnet/arcade/blob/d9405ccf3b5eba375b75ecbf38e3d2e090948e21/src/Microsoft.DotNet.Arcade.Sdk/tools/ProjectDefaults.props#L94

[danmoseley]

This compiler warning is disabled in Arcade :(

https://github.com/dotnet/arcade/blob/d9405ccf3b5eba375b75ecbf38e3d2e090948e21/src/Microsoft.DotNet.Arcade.Sdk/tools/ProjectDefaults.props#L94

we can just do <NoWarn>$(NoWarn.Replace('1591','')</NoWarn> globally for ourselves?

[ViktorHofer]

Technically yes, but the regarding the bigger picture, Arcade is about providing the right defaults. IMO we should discuss if that setting still makes sense (it comes from roslyn's toolset repo).

EDIT: I don't see this issue related to 7.0.0 in any way.

[ericstj]

I don't see this issue related to 7.0.0 in any way.

Moving this out to 8.0.0

[carlossanlop]

Update:

• The CS1591 issue in Arcade was addressed in PR: Make it possible to not globally suppress CS1591 arcade#12188
• We started using the new SkipArcadeNoWarnCS1591 property in the runtime repo with PR: Copy local intellisense xmls for assemblies with source of truth. #79134
• We have 3 assemblies that originally had triple slash set as their source of truth, but only one actually ships the built xml (explained here):
  • System.Formats.Cbor generates its own xml from triple slash and we already ship that to customers.
  • System.IO.Compression.Brotli can't yet use its own xml because it uses GeneratePlatformNotSupportedAssemblyMessage.
  • System.Numerics.Vectors can't yet use its own xml because it is marked as IsPartialFacadeAssembly.

I added <SkipArcadeNoWarnCS1591>false</SkipArcadeNoWarnCS1591> to src/System.Formats.Cbor.csproj and it works (build failure) if I delete an existing public triple slash line.

I'm submitting a PR to enable the rule in this one assembly. The issue would have to stay open until we address the same in all assemblies (eventually).

[carlossanlop]

I forgot that UseIntellisensePackageDocXmlFile already adds CS1591 to NoWarn for all assemblies that use the private intellisense package as source of truth, and all the others (like System.Formats.Cbor) have the rule already enabled.

That means we can close this issue since this is not a problem anymore. Assemblies can use their triple slash as source of truth by disabling the UseIntellisensePackageDocXmlFile property in their src csproj, and the rule will be automatically enabled.