Documentation crefs point to nonexisting classes

[markusweimer]

System information

• OS version/distro: Windows 10 v1803
• .NET Version (eg., dotnet --info): 2.1.201

Issue

• What did you do? I opened the solution in Jetbrains Rider.
• What happened? Rider pointed out a great many mistakes in the cref elements of XMLDocs
• What did you expect? Perfect documentation, of course :-)

[TomFinley]

Hi @markusweimer , generally Visual Studio warns us when the XML documentation points to something that does not exist -- for that reason I am surprised to hear that there is a problem in this area, since I'd expect the code to not compile. Could you post some examples of this, to let us know where things are going wrong?

[markusweimer]

Of course! Here are all the instances I found:

<src>\<Microsoft.ML.Core>\Environment\HostEnvironmentBase.cs:3209 Cannot resolve symbol 'StartPipe'
<src>\<Microsoft.ML.Core>\Utilities\VBufferUtils.cs:27610 Cannot resolve symbol 'ApplyWith'
<src>\<Microsoft.ML.Core>\Utilities\VBufferUtils.cs:51665 Cannot resolve symbol 'ApplyWith'
<src>\<Microsoft.ML.Core>\Utilities\VBufferUtils.cs:51693 Cannot resolve symbol 'ApplyWithEitherDefined'
<src>\<Microsoft.ML.Core>\Utilities\VBufferUtils.cs:51872 Cannot resolve symbol 'ApplyWith'
<src>\<Microsoft.ML.Core>\Utilities\VBufferUtils.cs:51912 Cannot resolve symbol 'ApplyWithEitherDefined'
<src>\<Microsoft.ML.Data>\DataView\Transposer.cs:68115 Cannot resolve symbol 'GetGetter'
<src>\<Microsoft.ML.Data>\Depricated\Vector\VBufferMathUtils.cs:14976 Cannot resolve symbol 'ApplyInto'
<src>\<Microsoft.ML.Data>\Scorers\BinaryClassifierScorer.cs:4245 Cannot resolve symbol 'CreateBound'
<src>\<Microsoft.ML.Data>\Scorers\MultiClassClassifierScorer.cs:21842 Cannot resolve symbol 'CreateBound'
<test>\<Microsoft.ML.TestFramework>\TestCommandBase.cs:13111 Cannot resolve symbol 'ProgressLogLine'
<test>\<Microsoft.ML.TestFramework>\TestCommandBase.cs:16933 Cannot resolve symbol 'ICommand'
<test>\<Microsoft.ML.TestFramework>\TestCommandBase.cs:18341 Cannot resolve symbol 'ICommand'
<test>\<Microsoft.ML.TestFramework>\TestCommandBase.cs:19453 Cannot resolve symbol 'ICommand'
<test>\<Microsoft.ML.TestFramework>\TestCommandBase.cs:20586 Cannot resolve symbol 'ICommand'
<test>\<Microsoft.ML.TestFramework>\TestCommandBase.cs:26565 Cannot resolve symbol 'RunContextBase'

[sharwell]

Copied from my comment in the pull request:

After further review, documentation comments are already validated during the build, and none of the locations changed by this pull request are ambiguous or problematic for the compiler. If a tool is failing to correctly read these references in source code, a bug should be filed for that tool because there is no validation we can automatically perform to ensure the comments stay "correct" with respect to those bugs in the future.

[markusweimer]

OK, so I played around some more in VS. Many of the cref with missing type parameters can be resolved in VS, and ctrl-click on them leads to the right place. Of course, the same is still true with the changes in this PR.

However, I cannot do the same in the cases where an import was missing. E.g. in TestCommandBase line 388, which references ICommand, which isn't imported in that file. This does work once I switch to the changes in this PR.

Hence, I am not convinced that our builds capture these issues completely. There seem to be three parsers for this, which offer different leniency or ability to resolve ambiguity:

1. ReSharper / Rider: least lenient (at least in its default config). They offer no heuristic to resolve ambiguous cref instances, even where there is clearly only one resolution.
2. Visual Studio: more lenient, and with clever heuristics for the case where type parameters are omitted.
3. Our builds: They either have an even more clever heuristic to guess the right import, or fail to identify the problem.

This change seems to be fixing several warnings for parser 1, some for parser 2 (although VS didn't warn about the broken cref) and be altogether benign to parser 3. Hence, I think this could be merged.

At the same time, we probably should file an issue about how our builds fail to detect some of these broken cref links. We might not be able to fix it, though.

[sharwell]

However, I cannot do the same in the cases where an import was missing. E.g. in TestCommandBase line 388, which references ICommand, which isn't imported in that file.

I'll get a pull request in to fix this case with validation during the build. It appears to have been overlooked in the previous work.

There seem to be three parsers for this ...

The second and third were supposed to be the same, but there is a configuration error that is currently
preventing that in test projects.