Do not report unused using/imports if not processing doc comments. #26425

gafter · 2018-04-26T19:05:12Z

Also silently process doc comments when parsing them so we can report unused usings.
Fixes #2773

Also silently process doc comments when parsing them so we can report unused usings. Fixes dotnet#2773

CyrusNajmabadi · 2018-04-26T19:50:19Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

@@ -1890,7 +1890,7 @@ internal override void ReportUnusedImports(SyntaxTree filterTree, DiagnosticBag
                    cancellationToken.ThrowIfCancellationRequested();

                    SyntaxTree infoTree = info.Tree;
-                    if (filterTree == null || filterTree == infoTree)
+                    if ((filterTree == null || filterTree == infoTree) && infoTree.Options.DocumentationMode != DocumentationMode.None)


just to check, this will not affect the IDE right? i.e. the user cannot select DocMode.None in the IDE?

otherwise, if they do, this change would mean they get no unused usings warnings. #Resolved

Correct, the IDE has never passed .None. It has always passed Parse or Diagnose depending on what the underlying flag is, precisely for this reason if I remember correctly.

roslyn/src/VisualStudio/CSharp/Test/ProjectSystemShim/LegacyProject/CSharpCompilerOptionsTests.cs

Lines 17 to 51 in 62bb55c

[WpfFact]

[Trait(Traits.Feature, Traits.Features.ProjectSystemShims)]

[WorkItem(530980, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/530980")]

public void DocumentationModeSetToDiagnoseIfProducingDocFile()

{

using (var environment = new TestEnvironment())

{

var project = CSharpHelpers.CreateCSharpProject(environment, "Test");

project.SetOptionWithMarshaledValue(CompilerOptions.OPTID_XML_DOCFILE, "DocFile.xml");

var workspaceProject = environment.Workspace.CurrentSolution.Projects.Single();

var options = (CSharpParseOptions)workspaceProject.ParseOptions;

Assert.Equal(DocumentationMode.Diagnose, options.DocumentationMode);

}

}

[WpfFact]

[Trait(Traits.Feature, Traits.Features.ProjectSystemShims)]

[WorkItem(530980, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/530980")]

public void DocumentationModeSetToParseIfNotProducingDocFile()

{

using (var environment = new TestEnvironment())

{

var project = CSharpHelpers.CreateCSharpProject(environment, "Test");

project.SetOptionWithMarshaledValue(CompilerOptions.OPTID_XML_DOCFILE, "");

var workspaceProject = environment.Workspace.CurrentSolution.Projects.Single();

var options = (CSharpParseOptions)workspaceProject.ParseOptions;

Assert.Equal(DocumentationMode.Parse, options.DocumentationMode);

}

}

#Resolved

Interesting. how does this fix the bug hten? We'll continue passing along the variable. But the change i see above in neal's work still won't do the analysis. So we'll still think the usings are unused... right? #Resolved

Correct: if the user selects DocMode.None, they will get no unused usings reported.

In reply to: 184510536 [](ancestors = 184510536)

We now do the analysis and drop the diagnostics on the floor.

In reply to: 184529384 [](ancestors = 184529384)

CyrusNajmabadi · 2018-04-27T00:26:29Z

src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs

@@ -413,13 +411,6 @@ public override void DefaultVisit(Symbol symbol)

                bool reportDiagnosticsForCurrentTrivia = trivia.SyntaxTree.ReportDocumentationCommentDiagnostics();


is this variable needed anymore? #Resolved

Yes, it is used later.

In reply to: 184566009 [](ancestors = 184566009)

CyrusNajmabadi · 2018-04-27T00:27:07Z

src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs

@@ -413,13 +411,6 @@ public override void DefaultVisit(Symbol symbol)

                bool reportDiagnosticsForCurrentTrivia = trivia.SyntaxTree.ReportDocumentationCommentDiagnostics();

-                // If we're writing XML or we need to report diagnostics (either in this particular piece of trivia,
-                // or concerning undocumented [type] parameters), then we need to process this trivia node.
-                if (!(haveWriter || reportDiagnosticsForCurrentTrivia || reportParameterOrTypeParameterDiagnostics))


can we remove reportParameterOrTypeParameterDiagnostics from the parameter list now? #Resolved

Used on line 337.

In reply to: 184566077 [](ancestors = 184566077)

CyrusNajmabadi · 2018-04-27T00:27:29Z

src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs

-                if (!(haveWriter || reportDiagnosticsForCurrentTrivia || reportParameterOrTypeParameterDiagnostics))
-                {
-                    continue;
-                }


Do we not need an equivalent VB change? DId it not have this problem? #Resolved

VB did not have this optimization, so part of the C# fix wasn't needed in VB.

In reply to: 184566119 [](ancestors = 184566119)

gafter · 2018-04-27T03:34:59Z

@dotnet/roslyn-compiler May I please have a couple of reviews of this number one most awaited bug fix?

AlekseyTs · 2018-04-27T16:57:39Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

@@ -1890,7 +1890,7 @@ internal override void ReportUnusedImports(SyntaxTree filterTree, DiagnosticBag
                    cancellationToken.ThrowIfCancellationRequested();

                    SyntaxTree infoTree = info.Tree;
-                    if (filterTree == null || filterTree == infoTree)
+                    if ((filterTree == null || filterTree == infoTree) && infoTree.Options.DocumentationMode != DocumentationMode.None)


infoTree.Options.DocumentationMode != DocumentationMode.None [](start = 74, length = 60)

Consider doing this check for filterTree early and bypassing the entire loop #Closed

I don't understand this suggestion.

In reply to: 184746390 [](ancestors = 184746390)

I don't understand this suggestion.

if filterTree is not null and its DocumentationMode is None, it looks like there is no reason to even doing the loop. I am suggesting to optimize for this situation.

In reply to: 184757896 [](ancestors = 184757896,184746390)

DocumentationMode is specific to each info.Tree.

In reply to: 184758853 [](ancestors = 184758853,184757896,184746390)

DocumentationMode is specific to each info.Tree.

But it looks like, if filterTree is not null, that is the only tree we are going to look at. Note, I am not suggesting to move the condition you added. I am suggesting to add an optimization in addition to what you have right now

In reply to: 184762886 [](ancestors = 184762886,184758853,184757896,184746390)

I still do not understand the proposed optimization. Even if filterTree is not null, there still may be zero, one, or many infos from that tree, which require a loop to iterate through.

In reply to: 184776699 [](ancestors = 184776699,184762886,184758853,184757896,184746390)

I still do not understand the proposed optimization. Even if filterTree is not null, there still may be zero, one, or many infos from that tree, which require a loop to iterate through.

Is this loop going to do anything useful if filterTree is not null and its DocumentationMode is None?

In reply to: 184786793 [](ancestors = 184786793,184776699,184762886,184758853,184757896,184746390)

Oh, that's what you were suggesting. Yes, that could be done.

AlekseyTs · 2018-04-27T17:07:17Z

src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb

@@ -1585,7 +1585,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
                    cancellationToken.ThrowIfCancellationRequested()

                    Dim infoTree As SyntaxTree = info.Tree
-                    If filterTree Is Nothing OrElse filterTree Is infoTree Then
+                    If (filterTree Is Nothing OrElse filterTree Is infoTree) AndAlso infoTree.Options.DocumentationMode <> DocumentationMode.None Then


infoTree.Options.DocumentationMode <> DocumentationMode.None [](start = 85, length = 60)

Same suggestion as for C#. #Closed

AlekseyTs · 2018-04-27T17:13:00Z

src/EditorFeatures/CSharpTest/RemoveUnnecessaryImports/RemoveUnnecessaryImportsTests.cs

-            // parsing doc comments as simple trivia; System is unnecessary
-            await TestInRegularAndScriptAsync(
+            // parsing doc comments as simple trivia; we don't know System is unnecessary
+            await TestMissingAsync(


TestMissingAsync [](start = 18, length = 16)

Are all three scenarios differ only by used DocumentationMode? Consider extracting extracting common parts to a method parameterized by DocumentationMode, or refactoring into a loop. #WontFix

AlekseyTs

LGTM (iteration 1)

gafter · 2018-04-28T00:18:55Z

@dotnet/roslyn-compiler May I please have a second review of this number one most anticipated bug fix?

AlekseyTs

LGTM (iteration 2)

sharwell · 2018-04-30T13:14:34Z

The solution appears to address the worst case for this side effects of this situation, but I'm concerned about the other cases that will come up:

Will users be prompted to add a using if it's missing from a cref?
Analyzers which deal with documentation comments still won't work unless documentation comment output is enabled, which means many of the downstream issues linked to this one will not be improved by this change.

Are we just going to close those as by design (or limited by API compatibility concerns)? What do we tell analyzer authors and users about avoiding cases where analyzers are enabled but do not behave correctly? For example, consider the impact of DocumentationMode on SA1626 (Single line comments must not use documentation style slashes).

gafter · 2018-05-02T16:43:46Z

@sharwell This change has no impact on those scenarios, and I do not intend to take any position on those issues.

gafter · 2018-05-02T16:46:16Z

@dotnet/roslyn-compiler May I please have a second review?

Do not report unused using/imports if not processing doc comments.

81f4249

Also silently process doc comments when parsing them so we can report unused usings. Fixes dotnet#2773

gafter added Bug Area-Compilers labels Apr 26, 2018

gafter added this to the 15.8 milestone Apr 26, 2018

gafter self-assigned this Apr 26, 2018

gafter requested review from a team as code owners April 26, 2018 19:05

CyrusNajmabadi reviewed Apr 26, 2018

View reviewed changes

CyrusNajmabadi reviewed Apr 27, 2018

View reviewed changes

AlekseyTs reviewed Apr 27, 2018

View reviewed changes

AlekseyTs approved these changes Apr 27, 2018

View reviewed changes

A slight optimization traversing the using declarations for warnings.

1bb3120

AlekseyTs approved these changes Apr 28, 2018

View reviewed changes

333fred approved these changes May 3, 2018

View reviewed changes

gafter merged commit a90d29b into dotnet:master May 3, 2018

gafter mentioned this pull request May 3, 2018

Usage analysis ignores references in documentation comments #2773

Closed

svick mentioned this pull request Oct 14, 2018

Incorrect determination of an unused using directive #30505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not report unused using/imports if not processing doc comments. #26425

Do not report unused using/imports if not processing doc comments. #26425

gafter commented Apr 26, 2018

CyrusNajmabadi Apr 26, 2018 •

edited by gafter

Loading

jasonmalinowski Apr 26, 2018 •

edited by gafter

Loading

CyrusNajmabadi Apr 26, 2018 •

edited by gafter

Loading

gafter Apr 27, 2018

gafter Apr 27, 2018

CyrusNajmabadi Apr 27, 2018 •

edited by gafter

Loading

gafter Apr 27, 2018

CyrusNajmabadi Apr 27, 2018 •

edited by gafter

Loading

gafter Apr 27, 2018

CyrusNajmabadi Apr 27, 2018 •

edited by gafter

Loading

gafter Apr 27, 2018

gafter commented Apr 27, 2018

AlekseyTs Apr 27, 2018 •

edited

Loading

gafter Apr 27, 2018

AlekseyTs Apr 27, 2018 •

edited

Loading

gafter Apr 27, 2018

AlekseyTs Apr 27, 2018

gafter Apr 27, 2018

AlekseyTs Apr 27, 2018

gafter Apr 27, 2018

AlekseyTs Apr 27, 2018 •

edited

Loading

AlekseyTs Apr 27, 2018 •

edited by gafter

Loading

AlekseyTs left a comment

gafter commented Apr 28, 2018 •

edited

Loading

AlekseyTs left a comment

sharwell commented Apr 30, 2018

gafter commented May 2, 2018

gafter commented May 2, 2018

	[WpfFact]
	[Trait(Traits.Feature, Traits.Features.ProjectSystemShims)]
	[WorkItem(530980, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/530980")]
	public void DocumentationModeSetToDiagnoseIfProducingDocFile()
	{
	using (var environment = new TestEnvironment())
	{
	var project = CSharpHelpers.CreateCSharpProject(environment, "Test");

	project.SetOptionWithMarshaledValue(CompilerOptions.OPTID_XML_DOCFILE, "DocFile.xml");

	var workspaceProject = environment.Workspace.CurrentSolution.Projects.Single();
	var options = (CSharpParseOptions)workspaceProject.ParseOptions;

	Assert.Equal(DocumentationMode.Diagnose, options.DocumentationMode);
	}
	}

	[WpfFact]
	[Trait(Traits.Feature, Traits.Features.ProjectSystemShims)]
	[WorkItem(530980, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/530980")]
	public void DocumentationModeSetToParseIfNotProducingDocFile()
	{
	using (var environment = new TestEnvironment())
	{
	var project = CSharpHelpers.CreateCSharpProject(environment, "Test");

	project.SetOptionWithMarshaledValue(CompilerOptions.OPTID_XML_DOCFILE, "");

	var workspaceProject = environment.Workspace.CurrentSolution.Projects.Single();
	var options = (CSharpParseOptions)workspaceProject.ParseOptions;

	Assert.Equal(DocumentationMode.Parse, options.DocumentationMode);
	}
	}

		@@ -413,13 +411,6 @@ public override void DefaultVisit(Symbol symbol)

		bool reportDiagnosticsForCurrentTrivia = trivia.SyntaxTree.ReportDocumentationCommentDiagnostics();

Do not report unused using/imports if not processing doc comments. #26425

Do not report unused using/imports if not processing doc comments. #26425

Conversation

gafter commented Apr 26, 2018

CyrusNajmabadi Apr 26, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jasonmalinowski Apr 26, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 26, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Apr 27, 2018

AlekseyTs Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

gafter commented Apr 28, 2018 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

sharwell commented Apr 30, 2018

gafter commented May 2, 2018

gafter commented May 2, 2018

CyrusNajmabadi Apr 26, 2018 •

edited by gafter

Loading

jasonmalinowski Apr 26, 2018 •

edited by gafter

Loading

CyrusNajmabadi Apr 26, 2018 •

edited by gafter

Loading

CyrusNajmabadi Apr 27, 2018 •

edited by gafter

Loading

CyrusNajmabadi Apr 27, 2018 •

edited by gafter

Loading

CyrusNajmabadi Apr 27, 2018 •

edited by gafter

Loading

AlekseyTs Apr 27, 2018 •

edited

Loading

AlekseyTs Apr 27, 2018 •

edited

Loading

AlekseyTs Apr 27, 2018 •

edited

Loading

AlekseyTs Apr 27, 2018 •

edited by gafter

Loading

gafter commented Apr 28, 2018 •

edited

Loading