Skip to content

Make DocumentationCommentCompiler work after NormalizeWhitespace#47360

Merged
jcouv merged 6 commits intodotnet:mainfrom
svick:normalizewhitespace-xmlcomments
Mar 23, 2021
Merged

Make DocumentationCommentCompiler work after NormalizeWhitespace#47360
jcouv merged 6 commits intodotnet:mainfrom
svick:normalizewhitespace-xmlcomments

Conversation

@svick
Copy link
Copy Markdown
Member

@svick svick commented Sep 2, 2020

Fixes #47278.

@svick svick requested a review from a team as a code owner September 2, 2020 08:57
@svick svick force-pushed the normalizewhitespace-xmlcomments branch from fd38951 to f8d7628 Compare September 2, 2020 09:38
@svick svick force-pushed the normalizewhitespace-xmlcomments branch from f8d7628 to ba3374e Compare September 2, 2020 09:38
@svick
Copy link
Copy Markdown
Member Author

svick commented Sep 2, 2020

When I look at the binlog of one of the failing builds, I see:

    Debug.Assert failed with message: Fail: If we hit this, then we might need to think about a different workaround for stripping the location out the message. 
    Stack Trace
       at Microsoft.CodeAnalysis.CommandLine.ExitingTraceListener.Exit(String originalMessage)
       at Microsoft.CodeAnalysis.CommandLine.ExitingTraceListener.WriteLine(String message)
       at System.Diagnostics.TraceListener.Fail(String message, String detailMessage)
       at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage)
       at System.Diagnostics.TraceInternal.TraceProvider.Fail(String message, String detailMessage)
       at Microsoft.CodeAnalysis.CSharp.DocumentationCommentCompiler.GetDescription(XmlException e)
       at Microsoft.CodeAnalysis.CSharp.DocumentationCommentCompiler.TryProcessDocumentationCommentTriviaNodes(Symbol symbol, Boolean isPartialMethodDefinitionPart, ImmutableArray`1 docCommentNodes, Boolean reportParameterOrTypeParameterDiagnostics, String& withUnprocessedIncludes, Boolean& haveParseError, HashSet`1& documentedTypeParameters, HashSet`1& documentedParameters, ImmutableArray`1& includeElementNodes)
       at Microsoft.CodeAnalysis.CSharp.DocumentationCommentCompiler.DefaultVisit(Symbol symbol)
       at Microsoft.CodeAnalysis.CSharp.DocumentationCommentCompiler.VisitNamedType(NamedTypeSymbol symbol)
       at Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol.Accept(CSharpSymbolVisitor visitor)
       at Microsoft.CodeAnalysis.CSharp.DocumentationCommentCompiler.VisitNamespace(NamespaceSymbol symbol)
       at Microsoft.CodeAnalysis.CSharp.Symbols.NamespaceSymbol.Accept(CSharpSymbolVisitor visitor)
       at Microsoft.CodeAnalysis.CSharp.DocumentationCommentCompiler.VisitNamespace(NamespaceSymbol symbol)
       at Microsoft.CodeAnalysis.CSharp.Symbols.NamespaceSymbol.Accept(CSharpSymbolVisitor visitor)
       at Microsoft.CodeAnalysis.CSharp.DocumentationCommentCompiler.VisitNamespace(NamespaceSymbol symbol)
       at Microsoft.CodeAnalysis.CSharp.Symbols.NamespaceSymbol.Accept(CSharpSymbolVisitor visitor)
       at Microsoft.CodeAnalysis.CSharp.DocumentationCommentCompiler.VisitNamespace(NamespaceSymbol symbol)
       at Microsoft.CodeAnalysis.CSharp.Symbols.NamespaceSymbol.Accept(CSharpSymbolVisitor visitor)
       at Microsoft.CodeAnalysis.CSharp.CSharpSymbolVisitor.Visit(Symbol symbol)
       at Microsoft.CodeAnalysis.CSharp.DocumentationCommentCompiler.WriteDocumentationCommentXml(CSharpCompilation compilation, String assemblyName, Stream xmlDocStream, DiagnosticBag diagnostics, CancellationToken cancellationToken, SyntaxTree filterTree, Nullable`1 filterSpanWithinTree)
       at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GenerateResourcesAndDocumentationComments(CommonPEModuleBuilder moduleBuilder, Stream xmlDocStream, Stream win32Resources, String outputNameOverride, DiagnosticBag diagnostics, CancellationToken cancellationToken)
       at Microsoft.CodeAnalysis.CommonCompiler.CompileAndEmit(TouchedFileLogger touchedFilesLogger, Compilation& compilation, ImmutableArray`1 analyzers, ImmutableArray`1 generators, ImmutableArray`1 additionalTextFiles, AnalyzerConfigSet analyzerConfigSet, ImmutableArray`1 sourceFileAnalyzerConfigOptions, ImmutableArray`1 embeddedTexts, DiagnosticBag diagnostics, CancellationToken cancellationToken, CancellationTokenSource& analyzerCts, Boolean& reportAnalyzer, AnalyzerDriver& analyzerDriver)
       at Microsoft.CodeAnalysis.CommonCompiler.RunCore(TextWriter consoleOutput, ErrorLogger errorLogger, CancellationToken cancellationToken)
       at Microsoft.CodeAnalysis.CommonCompiler.Run(TextWriter consoleOutput, CancellationToken cancellationToken)
       at Microsoft.CodeAnalysis.CSharp.CommandLine.Csc.<>c__DisplayClass1_0.<Run>b__0(TextWriter tw)
       at Microsoft.CodeAnalysis.CommandLine.ConsoleUtil.RunWithUtf8Output[T](Boolean utf8Output, TextWriter textWriter, Func`2 func)
       at Microsoft.CodeAnalysis.CSharp.CommandLine.Csc.Run(String[] args, BuildPaths buildPaths, TextWriter textWriter, IAnalyzerAssemblyLoader analyzerLoader)
       at Microsoft.CodeAnalysis.CommandLine.BuildClient.RunLocalCompilation(String[] arguments, BuildPaths buildPaths, TextWriter textWriter)
       at Microsoft.CodeAnalysis.CommandLine.BuildClient.RunCompilation(IEnumerable`1 originalArguments, BuildPaths buildPaths, TextWriter textWriter, String pipeName)
       at Microsoft.CodeAnalysis.CommandLine.BuildClient.Run(IEnumerable`1 arguments, RequestLanguage language, CompileFunc compileFunc)
       at Microsoft.CodeAnalysis.CSharp.CommandLine.Program.MainCore(String[] args)
       at Microsoft.CodeAnalysis.CSharp.CommandLine.Program.Main(String[] args)

I already encountered that assert in #47363, but I don't understand why it would be triggered by this PR.

@jcouv jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Sep 2, 2020
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Sep 2, 2020

but I don't understand why it would be triggered by this PR.

You're changing DocumentationCommentCompiler.WriteFormattedSingleLineComment, and the crash is in DocumentationCommentCompiler. So is it possible you've broken an invariant and are now causing a crash?

@svick svick force-pushed the normalizewhitespace-xmlcomments branch from 8759181 to e015bed Compare September 3, 2020 10:39
@svick svick force-pushed the normalizewhitespace-xmlcomments branch from e015bed to 365bf00 Compare September 3, 2020 10:40
@svick
Copy link
Copy Markdown
Member Author

svick commented Sep 3, 2020

@CyrusNajmabadi You're right, it was an ordinary bug in my code. I think what confused me is that it caused build error when the compiler compiles itself.

The current CI failure seems unrelated to this PR:

Downloading https://raw.githubusercontent.com/dotnet/roslyn/master/eng/config/PublishData.json
Unable to connect to the remote server
System.Net.WebException: Unable to connect to the remote server ---> System.Net.Sockets.SocketException: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond 151.101.188.133:443

@333fred
Copy link
Copy Markdown
Member

333fred commented Sep 3, 2020

You're right, it was an ordinary bug in my code

Please add a test that hits this bug so we can avoid having to actually build the compiler to validate it in the future :)

@svick
Copy link
Copy Markdown
Member Author

svick commented Sep 3, 2020 •
edited
Loading

@333fred Those tests already exist. For example, see this failure, where that bug incorrectly caused:

warning CS1570: XML comment has badly formed XML -- 'The 'summary' start tag on line 17 position 85 does not match the end tag of '_120872cc27f042ec90731c18be9f9edf'. Line 22, position 3.'

So I think additional tests for this are not necessary.

333fred
333fred reviewed Sep 3, 2020
View reviewed changes
@svick svick force-pushed the normalizewhitespace-xmlcomments branch from 98783bb to e6c5bf1 Compare September 4, 2020 09:37
@svick svick force-pushed the normalizewhitespace-xmlcomments branch from 5c45f3e to ec4a8fe Compare September 7, 2020 09:38
@333fred
Copy link
Copy Markdown
Member

333fred commented Sep 10, 2020

@dotnet/roslyn-compiler for a second review.

Base automatically changed from master to main March 3, 2021 23:52
Youssef1313
Youssef1313 reviewed Mar 19, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

From the linked issue:

It seems the call to NormalizeWhitespace() somehow turns the tree into a shape that is not supported by DocumentationCommentCompiler.

Shouldn't the fix be in NormalizeWhitespace so it produces a correct tree?

@333fred
Copy link
Copy Markdown
Member

333fred commented Mar 22, 2021

@dotnet/roslyn-compiler for a second review of this community PR please.

jcouv
jcouv approved these changes Mar 23, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 6)

@jcouv jcouv merged commit f554f62 into dotnet:main Mar 23, 2021
@ghost ghost added this to the Next milestone Mar 23, 2021
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 23, 2021

Merged/squashed. Thanks!

jaredpar added a commit that referenced this pull request Mar 23, 2021
@jaredpar
Revert "Make DocumentationCommentCompiler work after NormalizeWhitesp…
d09f248 
…ace (#47360)"

This reverts commit f554f62.
@jaredpar jaredpar mentioned this pull request Mar 23, 2021
Merged
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 23, 2021

Sorry, I didn't catch that the CI results were old. So this caused a build break when merged. Feel free to revert the revert commit in a new PR, merge with recent bits from main branch and submit again. Thanks

AlekseyTs
AlekseyTs reviewed Mar 23, 2021
View reviewed changes
src/Compilers/CSharp/Test/Syntax/LexicalAndXml/XmlDocCommentTests.cs
var text = @"namespace Foo
{
/// <summary>
/// This is Foo.Bar
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 23, 2021

Choose a reason for hiding this comment

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

Foo [](start = 16, length = 3)

"Foo" is getting flagged by policheck. Please remove usages of this word. from the tests.

@333fred 333fred mentioned this pull request Mar 25, 2021
Merged
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlekseyTs AlekseyTs AlekseyTs left review comments

@333fred 333fred 333fred approved these changes

@jcouv jcouv jcouv approved these changes

+1 more reviewer

@Youssef1313 Youssef1313 Youssef1313 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

16.10.P2

Development

Successfully merging this pull request may close these issues.

NormalizeWhitespace causes error on XML comments

7 participants

@svick @CyrusNajmabadi @333fred @jcouv @AlekseyTs @Youssef1313 @allisonchou