Usage analysis ignores references in documentation comments

[sharwell]

Migrated from CodePlex

Consider the following class.

namespace SampleNamespace
{
    using System;
    using System.Threading.Tasks;

    /// <summary>
    /// Some stuff is at
    /// <see cref="Task.ContinueWith(Action{Task, object}, object)"/>.
    /// </summary>
    public class SampleClass
    {
    }
}

The behavior of Organize Usings varies by whether or not the current project is configured to generate an XML documentation file with the output.

• If XML documentation file is enabled: The Organize Usings feature will not make any changes to the file shown above.
• If XML documentation file is disabled: The Organize Usings feature will remove both of the using declarations in the file shown above.

At least for the purpose of analyzing using declarations, the behavior of Roslyn should not depend on whether or not the project is currently configured to produce an XML documentation file during the build.

[srivatsn]

The "unused using" diagnostic is emitted by the compiler. It looks like the compiler is not even looking at doc comments when it doesn't need to generate a xml doc file and so doesn't include that code in it's analysis.

[Pilchie]

We have an option to "Diagnose" xml doc comments but not emit them, but that also triggers all the other warnings and errors, and so I don't think VS integration sets it.

We might need some finer grained control over the compiler's handling of xml doc comments. I remember discussing this with @amcasey a LONG time ago, but I don't think we resolved anything.

[amcasey]

See http://source.roslyn.io/#Microsoft.CodeAnalysis/DocumentationMode.cs

[matkoch]

+1

[Pilchie]

Hitcount++ from internal bug 1167079

[jnm2]

If removing the using breaks a cref, it should not be suggested. Please fix.

Currently this results in bouncing back and forth between ReSharper complaining that you should add the using for the XML documentation and Roslyn complaining that you should remove it. It's a bad user experience.

If the only correct solution is truly to enable XML doc generation, then please stop suggesting removal of the using and start suggesting enabling XML documentation output.

This may be a moonshot, but in an ideal world, I think I should be able to leave XML documentation output off and still have all the syntax checking as though it was on, minus the warning about publicly visible types missing comments.

[sharwell]

@svick are you able to test this on tunnelvisionlabs/dotnet-threading?

[breyed]

FWIW in thinking about the tradeoffs, the workflow I generally use is to configure debug builds to be as fast as possible, so I leave XML comments off. I rely on a release build, with all diagnostics, including static code analysis and XML comments, to be run often enough to catch deeper problems in a timely fashion.

It's OK for the diagnostic build to be slower. However, it's not OK when I see that the usings in a C# file have gotten jumbled and so press Ctrl+R, Ctrl+G (Remove and Sort Warnings), to subtly introduce warnings or errors into the next diagnostic build.

[gafter]

@dotnet/roslyn-ide @dotnet/roslyn-analysis @Pilchie @DustinCampbell I'm working on this issue now. My proposal is that the compiler will not produce any "unused using" diagnostics when the syntax trees are parsed with DocumentationMode.None, because in that case we have no way of knowing if they were used or not. I believe the IDE uses DocumentationMode.Parse, which gives the compiler enough information to determine if the usings are used or not, so this should not detract from the IDE experience. The IDE experience after my fix would be that we now properly recognize that usings (imports for VB) are used in doc comments.

Are you OK with this?

[jcouv]

@gafter That seems a worse experience, in my mind.
There was some analysis (above) of parsing the documentation in all cases (but without emitting extra files). It looked like the performance hit was minimal. Why not do that?

[jnm2]

I think the same way as @jcouv.

[gafter]

There was some analysis (above) of parsing the documentation in all cases (but without emitting extra files). It looked like the performance hit was minimal. Why not do that?

Because we get a different syntax tree, and that is a compat break for an unknown number of clients. Are you suggesting that we Obsolete DocumentationMode.None, which parses doc comments as ordinary comments?

[mavasani]

I am +1 on @gafter's proposal as it retains the existing API and parse semantics for all documentation comment modes, does not increase the command line build time (however negligible) and improves the user experience for IDE scenario.

API and parse semantics are very explicit on the DocumentationMode enum:

    public enum DocumentationMode : byte
    {
        /// <summary>
        /// Treats documentation comments as regular comments.
        /// </summary>
        None = 0,
 
        /// <summary>
        /// Parses documentation comments as structured trivia, but do not report any diagnostics.
        /// </summary>
        Parse = 1,
 
        /// <summary>
        /// Parses documentation comments as structured trivia and report diagnostics.
        /// </summary>
        Diagnose = 2,
    }

None should parse doc comments as regular comments, which means unused using diagnostics can never be produced reliably. IDE does not use this mode and command line compilation would have never output these diagnostics as they have hidden severity. This is also likely the most commonly used mode in command line compilation, and I am not sure if it is worth risking any performance hit here by trying to parse documentation comments.

Parse should parse documentation comments, and not report any documentation comment diagnostics. We should fix this mode to correctly consume documentation comments for determining unused usings. I don't think this can realistically break any external API consumers, as we not going to produce any new diagnostics, just stop producing incorrect unused using diagnostics. Hence, I don't think we need any new documentation mode such as Bind.

[gafter]

Fixed in #26425

[Pilchie]

Hurray! Finally! Thanks @gafter!