Add potential exceptions when generating doc comment #54894

Youssef1313 · 2021-07-17T11:21:08Z

CyrusNajmabadi · 2021-07-17T18:18:56Z

src/Features/CSharp/Portable/DocumentationComments/DocumentationCommentSnippetService.cs

+
+                if (expression is ObjectCreationExpressionSyntax { Type: TypeSyntax type })
+                {
+                    yield return type.ToString();


Likely won't work with generics. But that's likely so niche as to not be relevant.

Is also like it if we filtered it exceptions in an inner scope that are caught before escaping.

Youssef1313 · 2021-07-17T21:09:00Z

...ternalAccess/OmniSharp/DocumentationComments/OmniSharpDocumentationCommentsSnippetService.cs

@@ -13,7 +14,7 @@ namespace Microsoft.CodeAnalysis.ExternalAccess.OmniSharp.DocumentationComments
 {
    internal static class OmniSharpDocumentationCommentsSnippetService


@333fred Can you take a look at the breaking changes for OmniSharp here?

Should be fine, these are already used in an async method. @JoeRobich and @filipw, fyi for the next time we upgrade Roslyn we'll need to make a small change here (and yay, the EA assembly is working as expected!).

nice, thanks!

Youssef1313

@CyrusNajmabadi This is lacking tests and few TODOs. But should be good to re-review.

I'll continue the work on it tomorrow (it's almost midnight here).

Edit: C# work is done.

CyrusNajmabadi · 2021-07-19T16:10:08Z

...ures/Core/Implementation/DocumentationComments/AbstractDocumentationCommentCommandHandler.cs

-        private static DocumentationCommentSnippet? InsertOnCharacterTyped(IDocumentationCommentSnippetService service, SyntaxTree syntaxTree, SourceText text, int position, DocumentOptionSet options, CancellationToken cancellationToken)
-            => service.GetDocumentationCommentSnippetOnCharacterTyped(syntaxTree, text, position, options, cancellationToken);
+        private static DocumentationCommentSnippet? InsertOnCharacterTyped(IDocumentationCommentSnippetService service, SyntaxTree syntaxTree, SourceText text, int position, DocumentOptionSet options, SemanticModel model, CancellationToken cancellationToken)
+            => service.GetDocumentationCommentSnippetOnCharacterTyped(syntaxTree, text, position, options, model, cancellationToken);


no need to pass a semantic model and a syntax tree. thel atter comes with the former.

CyrusNajmabadi · 2021-07-19T16:10:33Z

src/Features/CSharp/Portable/DocumentationComments/DocumentationCommentSnippetService.cs

+        {
+            var throwExpressionsAndStatements = member.DescendantNodes().Where(n => n.IsKind(SyntaxKind.ThrowExpression, SyntaxKind.ThrowStatement));
+            var namespacesInScope = model.GetUsingNamespacesInScope(member);
+            var hasUsingSystem = namespacesInScope.Contains(n => n.ContainingNamespace.IsGlobalNamespace && n.Name == "System");


this needs doc as to why you're doing this.

CyrusNajmabadi · 2021-07-19T16:10:50Z

src/Features/CSharp/Portable/DocumentationComments/DocumentationCommentSnippetService.cs

+        {
+            var throwExpressionsAndStatements = member.DescendantNodes().Where(n => n.IsKind(SyntaxKind.ThrowExpression, SyntaxKind.ThrowStatement));
+            var namespacesInScope = model.GetUsingNamespacesInScope(member);
+            var hasUsingSystem = namespacesInScope.Contains(n => n.ContainingNamespace.IsGlobalNamespace && n.Name == "System");


Suggested change

var hasUsingSystem = namespacesInScope.Contains(n => n.ContainingNamespace.IsGlobalNamespace && n.Name == "System");

var hasUsingSystem = namespacesInScope.Contains(n => n.ContainingNamespace.IsGlobalNamespace && n.Name == nameof(System));

CyrusNajmabadi · 2021-07-19T16:13:46Z

src/Features/CSharp/Portable/DocumentationComments/DocumentationCommentSnippetService.cs

+
+        private static bool IsExceptionCaughtAndNotRethrown(SyntaxNode throwExpressionOrStatement, ITypeSymbol exceptionType, SemanticModel model, CancellationToken cancellationToken)
+        {
+            var tryStatement = throwExpressionOrStatement.FirstAncestorOrSelfUntil<TryStatementSyntax>(n => n is CatchClauseSyntax or LocalFunctionStatementSyntax or MemberDeclarationSyntax);


i think you'd need to walk up all containing try statements, not jsut the immediate one.

CyrusNajmabadi · 2021-07-19T16:14:02Z

src/Features/CSharp/Portable/DocumentationComments/DocumentationCommentSnippetService.cs

            }

            return list;
        }

+        private static IEnumerable<string> GetExceptions(SyntaxNode member, SemanticModel model, OptionSet options, CancellationToken cancellationToken)
+        {
+            var throwExpressionsAndStatements = member.DescendantNodes().Where(n => n.IsKind(SyntaxKind.ThrowExpression, SyntaxKind.ThrowStatement));


this will find in nested lambdas/localfunctions

Finding in nested local functions (possible lambdas too) is okay since they're expected to be called within the method. However, a try catch wrapping the local function call seems tricky to handle. Is this an important scenario?

i could see the argument about nested local functions (and even the it's iffy). not lambdas though. I would personally not handle either unless you can actually see the local function being called (as opposed to passed as a delegate).

and i do think we should basically be doing this analysis as finding the locations in the method where exceptions can be thrown, but then filtering those out if they are contained ina try/catch that would catch that particular exception.

this comment still applies.

CyrusNajmabadi · 2021-07-19T16:15:59Z

...ures/Core/Implementation/DocumentationComments/AbstractDocumentationCommentCommandHandler.cs

@@ -92,8 +92,9 @@ private static void ApplySnippet(DocumentationCommentSnippet snippet, ITextBuffe
            var syntaxTree = document.GetRequiredSyntaxTreeSynchronously(cancellationToken);
            var text = syntaxTree.GetText(cancellationToken);
            var documentOptions = document.GetOptionsAsync(cancellationToken).WaitAndGetResult(cancellationToken);
+            var semanticModel = document.GetRequiredSemanticModelAsync(cancellationToken).AsTask().WaitAndGetResult(cancellationToken);


getting the semantic model may be expensive. to mitigate this we should get a document with frozen-partial-semantics.

maxkoshevoi · 2021-07-21T08:17:12Z

Could you also apply this when generating documentation for interface member with single implementation?

333fred

O# EA layer changes LGTM.

CyrusNajmabadi · 2021-07-28T17:52:24Z

...ures/Core/Implementation/DocumentationComments/AbstractDocumentationCommentCommandHandler.cs

            var documentOptions = document.GetOptionsAsync(cancellationToken).WaitAndGetResult(cancellationToken);
-
-            var snippet = getSnippetAction(service, syntaxTree, text, caretPosition, documentOptions, cancellationToken);
+            var semanticModel = document.GetPartialSemanticModelAsync(cancellationToken).WaitAndGetResult(cancellationToken);


tbh, this is just incredibly concerning. semantic models (and the work therin) is not cheap, and this is a sync codepath. do we need semantics? can't we look for throws purely syntactically?

CyrusNajmabadi · 2021-07-28T17:53:33Z

src/Features/CSharp/Portable/DocumentationComments/DocumentationCommentSnippetService.cs

+            var throwExpressionsAndStatements = member.DescendantNodes().Where(n => n.IsKind(SyntaxKind.ThrowExpression, SyntaxKind.ThrowStatement));
+            var namespacesInScope = model.GetUsingNamespacesInScope(member);
+            // We generate <exception cref="System.NullReferenceException"> if there is no 'using System;', and generate
+            // <exception cref="NullReferenceException"> if there is a 'using System;'


this feels unnecessary. we can just look at how the throw looked. if teh user says throw NullReferenceException then we generate the same. if they say throw System.NullReferenceException likewise.

CyrusNajmabadi · 2021-07-28T17:54:59Z

src/Features/CSharp/Portable/DocumentationComments/DocumentationCommentSnippetService.cs

+
+                if (model.GetOperation(catchClause, cancellationToken) is ICatchClauseOperation catchClauseOperation)
+                {
+                    if (exceptionType.InheritsFromOrEquals(catchClauseOperation.ExceptionType))


i see. you use semantics for this. i'm ok with this doing a simple thing and just comparing the last portion of the name for equality. it will be more than good enough and will avoid semantics.

CyrusNajmabadi · 2021-07-28T17:55:31Z

@Youssef1313 i can make thsee changes if you'd like.

Youssef1313 · 2021-07-28T18:04:32Z

@Youssef1313 i can make thsee changes if you'd like.

That would be awesome and much appreciated! I didn't get enough time for this. Thanks!

…ents

Add potential exceptions when generating doc comment

dbd3cba

Youssef1313 requested a review from a team as a code owner July 17, 2021 11:21

dotnet-issue-labeler bot added the Area-IDE label Jul 17, 2021

handle constructors

e491eb4

CyrusNajmabadi reviewed Jul 17, 2021

View reviewed changes

More work

2101fc7

Youssef1313 requested a review from a team as a code owner July 17, 2021 21:06

Youssef1313 commented Jul 17, 2021

View reviewed changes

Youssef1313 requested review from CyrusNajmabadi and removed request for a team July 17, 2021 21:11

Youssef1313 added 2 commits July 18, 2021 10:33

Remaining work

d8f3033

Fix test

469fb79

jinujoseph assigned CyrusNajmabadi Jul 19, 2021

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jul 19, 2021

CyrusNajmabadi reviewed Jul 19, 2021

View reviewed changes

Address part of feedback

8525242

333fred approved these changes Jul 23, 2021

View reviewed changes

CyrusNajmabadi reviewed Jul 28, 2021

View reviewed changes

CyrusNajmabadi added 4 commits July 28, 2021 11:27

Merge remote-tracking branch 'upstream/main' into exceptions-doc-comm…

69e89da

…ents

Work

f24f8a9

Only use syntax

a395d1d

restore

c24f3c4

CyrusNajmabadi merged commit ac28a69 into dotnet:main Jul 28, 2021

ghost added this to the Next milestone Jul 28, 2021

Youssef1313 deleted the exceptions-doc-comments branch July 29, 2021 05:22

jinujoseph added the Feature Request label Jul 29, 2021

dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add potential exceptions when generating doc comment #54894

Add potential exceptions when generating doc comment #54894

Youssef1313 commented Jul 17, 2021 •

edited

Loading

CyrusNajmabadi Jul 17, 2021

Youssef1313 Jul 17, 2021

333fred Jul 23, 2021

filipw Jul 23, 2021

Youssef1313 left a comment •

edited

Loading

CyrusNajmabadi Jul 19, 2021

CyrusNajmabadi Jul 19, 2021

CyrusNajmabadi Jul 19, 2021

CyrusNajmabadi Jul 19, 2021

CyrusNajmabadi Jul 19, 2021

Youssef1313 Jul 21, 2021

CyrusNajmabadi Jul 21, 2021

CyrusNajmabadi Jul 21, 2021

CyrusNajmabadi Jul 28, 2021

CyrusNajmabadi Jul 19, 2021

maxkoshevoi commented Jul 21, 2021

333fred left a comment

CyrusNajmabadi Jul 28, 2021

CyrusNajmabadi Jul 28, 2021

CyrusNajmabadi Jul 28, 2021

CyrusNajmabadi commented Jul 28, 2021

Youssef1313 commented Jul 28, 2021

		@@ -13,7 +14,7 @@ namespace Microsoft.CodeAnalysis.ExternalAccess.OmniSharp.DocumentationComments
		{
		internal static class OmniSharpDocumentationCommentsSnippetService

	var hasUsingSystem = namespacesInScope.Contains(n => n.ContainingNamespace.IsGlobalNamespace && n.Name == "System");
	var hasUsingSystem = namespacesInScope.Contains(n => n.ContainingNamespace.IsGlobalNamespace && n.Name == nameof(System));

Add potential exceptions when generating doc comment #54894

Add potential exceptions when generating doc comment #54894

Conversation

Youssef1313 commented Jul 17, 2021 • 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

Youssef1313 left a comment • 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

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

maxkoshevoi commented Jul 21, 2021

333fred left a 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

CyrusNajmabadi commented Jul 28, 2021

Youssef1313 commented Jul 28, 2021

Youssef1313 commented Jul 17, 2021 •

edited

Loading

Youssef1313 left a comment •

edited

Loading