Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
May 3, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81f4249
Do not report unused using/imports if not processing doc comments.
gafter Apr 26, 2018
1bb3120
A slight optimization traversing the using declarations for warnings.
gafter Apr 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1883,14 +1883,14 @@ private void CompleteTree(SyntaxTree tree)

internal override void ReportUnusedImports(SyntaxTree filterTree, DiagnosticBag diagnostics, CancellationToken cancellationToken)
{
if (_lazyImportInfos != null)
if (_lazyImportInfos != null && filterTree?.Options.DocumentationMode != DocumentationMode.None)
{
foreach (ImportInfo info in _lazyImportInfos)
{
cancellationToken.ThrowIfCancellationRequested();

SyntaxTree infoTree = info.Tree;
if (filterTree == null || filterTree == infoTree)
if ((filterTree == null || filterTree == infoTree) && infoTree.Options.DocumentationMode != DocumentationMode.None)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 26, 2018 •
edited by gafter
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jasonmalinowski jasonmalinowski Apr 26, 2018 •
edited by gafter
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 26, 2018 •
edited by gafter
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this suggestion.

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

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DocumentationMode is specific to each info.Tree.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
TextSpan infoSpan = info.Span;
if (!this.IsImportDirectiveUsed(infoTree, infoSpan.Start))
Expand Down
9 changes: 0 additions & 9 deletions src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,6 @@ private bool TryProcessDocumentationCommentTriviaNodes(
{
Debug.Assert(!docCommentNodes.IsDefaultOrEmpty);

bool haveWriter = _writer != null;

bool processedDocComment = false; // Even if there are DocumentationCommentTriviaSyntax, we may not need to process any of them.

ArrayBuilder<CSharpSyntaxNode> includeElementNodesBuilder = null;
Expand All @@ -413,13 +411,6 @@ private bool TryProcessDocumentationCommentTriviaNodes(

bool reportDiagnosticsForCurrentTrivia = trivia.SyntaxTree.ReportDocumentationCommentDiagnostics();
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 27, 2018 •
edited by gafter
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this variable needed anymore? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is used later.

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


// 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))
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 27, 2018 •
edited by gafter
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used on line 337.

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

{
continue;
}
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 27, 2018 •
edited by gafter
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


if (!processedDocComment)
{
// Since we have to throw away all the parts if any part is bad, we need to write to an intermediate temp.
Expand Down
37 changes: 31 additions & 6 deletions src/Compilers/CSharp/Test/Symbol/Compilation/GetUnusedImportDirectivesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,12 +329,8 @@ public void CrefCountsAsUse()
/// <see cref='Console'/>
public class C { }
";

// Not binding doc comments.
CreateCompilation(source).VerifyDiagnostics(
// (2,1): info CS8019: Unnecessary using directive.
// using System;
Diagnostic(ErrorCode.HDN_UnusedUsingDirective, "using System;"));
// Not reporting doc comment diagnostics. It is still a use.
CreateCompilation(source).VerifyDiagnostics();

// Binding doc comments.
CreateCompilationWithMscorlib40AndDocumentationComments(source).VerifyDiagnostics();
Expand Down Expand Up @@ -430,5 +426,34 @@ partial void M(ref int x) { }
//Diagnostic(ErrorCode.HDN_UnusedUsingDirective, "using System.Runtime.InteropServices;").WithLocation(1, 1)
);
}

[Fact, WorkItem(2773, "https://github.com/dotnet/roslyn/issues/2773")]
public void UsageInDocComment()
{
var source = @"using X;

/// <summary/>
public class Program
{
/// <summary>
/// <see cref=""Q""/>
/// </summary>
static void Main(string[] args)
{
}
}

namespace X
{
/// <summary/>
public class Q { }
}
";
foreach (DocumentationMode documentationMode in Enum.GetValues(typeof(DocumentationMode)))
{
var compilation = CreateCompilation(source, parseOptions: TestOptions.Regular.WithDocumentationMode(documentationMode));
compilation.VerifyDiagnostics();
}
}
}
}
8 changes: 4 additions & 4 deletions src/Compilers/CSharp/Test/Symbol/DocumentationComments/CrefTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4369,17 +4369,17 @@ public partial class D { }
public partial class E { }
";

var tree1 = Parse(source1, options: TestOptions.RegularWithDocumentationComments.WithLanguageVersion(LanguageVersion.Latest));
var tree2 = Parse(source2, options: TestOptions.Regular.WithLanguageVersion(LanguageVersion.Latest));
var tree1 = Parse(source1, options: TestOptions.Regular.WithDocumentationMode(DocumentationMode.Diagnose).WithLanguageVersion(LanguageVersion.Latest));
var tree2 = Parse(source2, options: TestOptions.Regular.WithDocumentationMode(DocumentationMode.None).WithLanguageVersion(LanguageVersion.Latest));

// This scenario does not exist in dev11, but the diagnostics seem reasonable.
CreateCompilation(new[] { tree1, tree2 }).VerifyDiagnostics(
// (5,22): warning CS1591: Missing XML comment for publicly visible type or member 'D'
// public partial class D { }
Diagnostic(ErrorCode.WRN_MissingXMLComment, "D").WithArguments("D"),
Diagnostic(ErrorCode.WRN_MissingXMLComment, "D").WithArguments("D").WithLocation(5, 22),
// (7,22): warning CS1591: Missing XML comment for publicly visible type or member 'E'
// public partial class E { }
Diagnostic(ErrorCode.WRN_MissingXMLComment, "E").WithArguments("E"));
Diagnostic(ErrorCode.WRN_MissingXMLComment, "E").WithArguments("E").WithLocation(7, 22));
}

[Fact]
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/Test/Utilities/CSharp/TestOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ namespace Microsoft.CodeAnalysis.CSharp.Test.Utilities
{
public static class TestOptions
{
// Disable documentation comments by default so that we don't need to
// Disable diagnosing documentation comments by default so that we don't need to
// document every public member of every test input.
public static readonly CSharpParseOptions Regular = new CSharpParseOptions(kind: SourceCodeKind.Regular, documentationMode: DocumentationMode.None).WithLanguageVersion(LanguageVersion.Latest);
public static readonly CSharpParseOptions Regular = new CSharpParseOptions(kind: SourceCodeKind.Regular, documentationMode: DocumentationMode.Parse).WithLanguageVersion(LanguageVersion.Latest);
public static readonly CSharpParseOptions Script = Regular.WithKind(SourceCodeKind.Script);
public static readonly CSharpParseOptions Regular6 = Regular.WithLanguageVersion(LanguageVersion.CSharp6);
public static readonly CSharpParseOptions Regular7 = Regular.WithLanguageVersion(LanguageVersion.CSharp7);
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1578,14 +1578,14 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
End Property

Friend Overrides Sub ReportUnusedImports(filterTree As SyntaxTree, diagnostics As DiagnosticBag, cancellationToken As CancellationToken)
If _lazyImportInfos IsNot Nothing Then
If _lazyImportInfos IsNot Nothing AndAlso (filterTree Is Nothing OrElse filterTree.Options.DocumentationMode <> DocumentationMode.None) Then
Dim unusedBuilder As ArrayBuilder(Of TextSpan) = Nothing

For Each info As ImportInfo In _lazyImportInfos
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
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Same suggestion as for C#. #Closed

Dim clauseSpans = info.ClauseSpans
Dim numClauseSpans = clauseSpans.Length

Expand Down
47 changes: 38 additions & 9 deletions src/Compilers/VisualBasic/Test/Semantic/Semantics/GetUnusedImportDirectivesTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,19 +281,17 @@ Imports System
''' <see cref='Console'/>
Class A
End Class
]]></file>
]]></file>
</compilation>

' Without doc comments.
CreateCompilationWithMscorlib40(source, parseOptions:=New VisualBasicParseOptions(documentationMode:=DocumentationMode.None)).AssertTheseDiagnostics(
<errors>
BC50001: Unused import statement.
Imports System
~~~~~~~~~~~~~~
</errors>, suppressInfos:=False)
CreateCompilationWithMscorlib40(source, parseOptions:=New VisualBasicParseOptions(documentationMode:=DocumentationMode.None)).AssertNoDiagnostics(suppressInfos:=False)

' With doc comments parsed.
CreateCompilationWithMscorlib40(source, parseOptions:=New VisualBasicParseOptions(documentationMode:=DocumentationMode.Parse)).AssertNoDiagnostics(suppressInfos:=False)

' With doc comments.
CreateCompilationWithMscorlib40(source, parseOptions:=New VisualBasicParseOptions(documentationMode:=DocumentationMode.Diagnose)).AssertTheseDiagnostics(<errors></errors>, suppressInfos:=False)
' With doc comments diagnosed.
CreateCompilationWithMscorlib40(source, parseOptions:=New VisualBasicParseOptions(documentationMode:=DocumentationMode.Diagnose)).AssertNoDiagnostics(suppressInfos:=False)
End Sub

<Fact>
Expand Down Expand Up @@ -366,5 +364,36 @@ End Class
Dim diagnostics = model.GetDiagnostics()
AssertTheseDiagnostics(diagnostics, <errors></errors>)
End Sub

<Fact, WorkItem(2773, "https://github.com/dotnet/roslyn/issues/2773")>
Public Sub UsageInDocComment()
Dim source =
<compilation>
<file name="a.vb"><![CDATA[
Imports X

''' <summary/>
Public Class Program

''' <summary>
''' <see cref="Q"/>
''' </summary>
Public Sub Main()
End Sub

End Class

Namespace Global.X
''' <summary/>
Public Class Q
End Class
End Namespace
]]></file>
</compilation>
For Each documentationMode As DocumentationMode In [Enum].GetValues(GetType(DocumentationMode))
Dim compilation = CreateCompilationWithMscorlib40(source, parseOptions:=TestOptions.Regular.WithDocumentationMode(documentationMode))
compilation.AssertNoDiagnostics(suppressInfos:=False)
Next
End Sub
End Class
End Namespace
16 changes: 10 additions & 6 deletions src/EditorFeatures/CSharpTest/RemoveUnnecessaryImports/RemoveUnnecessaryImportsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1307,19 +1307,23 @@ public static void Main()
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryImports)]
public async Task TestReferenceInCref()
{
// 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(
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 27, 2018 •
edited by gafter
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@"[|using System;
/// <summary><see cref=""String"" /></summary>
class C
{
}|]",
@"/// <summary><see cref=""String"" /></summary>
}|]", new TestParameters(Options.Regular.WithDocumentationMode(DocumentationMode.None)));

// fully parsing doc comments; System is necessary
await TestMissingAsync(
@"[|using System;
/// <summary><see cref=""String"" /></summary>
class C
{
}");
}|]", new TestParameters(Options.Regular.WithDocumentationMode(DocumentationMode.Parse)));

// fully parsing doc comments; System is necessary
// fully parsing and diagnosing doc comments; System is necessary
await TestMissingAsync(
@"[|using System;
/// <summary><see cref=""String"" /></summary>
Expand Down