Make SA1600 trigger on undocumented record primary ctor parameters #3783

SapiensAnatis · 2024-01-22T22:21:48Z

Add analysis of XML <param> tags to SA1600 so that it is raised on record primary constructor parameters. Closes #3780.

This is in draft as I had a few questions:

All of my diagnostics are being reported twice in the C# 9 unit tests. I've baked these into the expectation, which is fine for C# 9 and C# 10, but causes C# 11 and C# 12 to fail after reporting the correct number of diagnostics.
I've added a diagnostic verifier method that passes custom XML files to the C# 9 unit test file, rather than the base unit tests, because the base SA1600 tests pass in this.LanguageVersion to the verification. I wasn't quite sure how (or if) I should adapt the custom verifier method to accept this.
The logic in SA1600ElementsMustBeDocumented.GetDocumentedParameters is more or less entirely copied from ElementDocumentationBase.HandleDeclaration. Would it be worth extracting this into a common helper, or are they sufficiently unique to leave separate?

Edit: I think all 3 items above are fairly resolved, so have moved out of draft.

Add analysis of XML <param> tags to SA1600 so that it is raised on record primary constructor parameters.

codecov · 2024-01-22T22:49:22Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f5f4ca9) 95.03% compared to head (bcdc55b) 97.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3783      +/-   ##
==========================================
+ Coverage   95.03%   97.43%   +2.40%     
==========================================
  Files         919      921       +2     
  Lines      109458   109774     +316     
  Branches     3306     3316      +10     
==========================================
+ Hits       104024   106960    +2936     
+ Misses       4455     1838    -2617     
+ Partials      979      976       -3

bjornhellander · 2024-01-23T06:41:54Z

You can add a virtual method in the test class to handle different behavior in different Roslyn versions.
See for example TestPositionalRecord1Async. I think it handles the exact same case.

bjornhellander · 2024-01-23T06:56:11Z

I've added a diagnostic verifier method that passes custom XML files to the C# 9 unit test file, rather than the base unit tests, because the base SA1600 tests pass in this.LanguageVersion to the verification. I wasn't quite sure how (or if) I should adapt the custom verifier method to accept this.

Sorry, but I didn't understand what you meant. Note that the verifier class contains a number of overloads of VerifyCSharpDiagnosticAsync, with different parameters, with and without language version. Why did you need to add this method?

SapiensAnatis · 2024-01-23T09:24:08Z

Why did you need to add this method?

I added it to pass in the xml files so that I could test the included documentation case, following the example of SA1611. Is there a different way to test this?

bjornhellander · 2024-01-23T10:23:52Z

Why did you need to add this method?

I added it to pass in the xml files so that I could test the included documentation case, following the example of SA1611. Is there a different way to test this?

Sorry! I completely misread your original comment and that's why I didn't understand it. I focused on your mention of LanguageVersion and got confused. Never mind. Adding it in that file seems fine to me. It can be moved later on if needed.

I will try to look at this again tonight, unless you have already received another response by then.

bjornhellander · 2024-01-23T20:34:16Z

StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs

+
+internal partial struct RecordDeclarationSyntaxWrapper : ISyntaxWrapper<TypeDeclarationSyntax>
+{
+    public static implicit operator RecordDeclarationSyntaxWrapper(TypeDeclarationSyntax node)


This corresponds to a conversion from a base type to a sub type. Adding an implicit conversion operator for that is not a good thing in my book.

I see your point, I am happy to make it an explicit operator. With the way it is being used I could also do something like:

bool RecordDeclarationSyntaxWrapper.TryWrap(TypeDeclarationSyntax node, out RecordDeclarationSyntaxWrapper result)

similar to int.TryParse and friends. I'm not sure if that's a bit out there though!

Just to be clear: The explicit conversion operator already exists, so you can just remove this file and add a cast where you get an error.

Ah, understood. Removed this file and used the explicit operator.

bjornhellander · 2024-01-23T20:43:51Z

StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs

+
+                    if (includedDocumentation.Nodes().OfType<XElement>().Any(element => element.Name == XmlCommentHelper.InheritdocXmlTag))
+                    {
+                        hasInheritdoc = true;


This code is not executed in tests. I have no idea why it was needed where you copied it from :-), but maybe you can try to find out to see if more tests should be added.

It handles the case where your XML comment includes an XML file, which itself specifies an <inheritdoc />.

I've added a test that should cover this case.

bjornhellander · 2024-01-23T20:55:01Z

StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs

+                }
+            }
+
+            private static class EmptyArray<T>


I would move it to the Helpers folder so it can be used in other places as well if needed

I did a bit of refactoring and was no longer using this, so I've removed it

bjornhellander · 2024-01-23T21:12:33Z

The logic in SA1600ElementsMustBeDocumented.GetDocumentedParameters is more or less entirely copied from ElementDocumentationBase.HandleDeclaration. Would it be worth extracting this into a common helper, or are they sufficiently unique to leave separate?

I think it's sufficiently complex to unify if it can be done without too much work, but I would wait for @sharwell's opinion on that.

SapiensAnatis · 2024-01-23T22:08:40Z

Just a quick question: is there any issue with me responding to review feedback now, knowing my pipelines will fail before the double diagnostic problem is resolved? I am a bit clueless there and @sharwell kindly offered to look into it in #3780.

I'm not familiar with Azure Pipelines and am unsure if you guys are being charged per run or anything like that 😅

bjornhellander · 2024-01-24T06:14:15Z

Just a quick question: is there any issue with me responding to review feedback now, knowing my pipelines will fail before the double diagnostic problem is resolved? I am a bit clueless there and @sharwell kindly offered to look into it in #3780.

I'm not familiar with Azure Pipelines and am unsure if you guys are being charged per run or anything like that 😅

I think that this will do it:

You can add a virtual method in the test class to handle different behavior in different Roslyn versions.
See for example TestPositionalRecord1Async. I think it handles the exact same case.

I just checked your failing tests and it looks to me like this should work. So instead of adding the expected diagnostics inside the test method, extract that code to a method per test (GetExpectedResult<TestName>) and override that one in every test project where the compiler behavior changes. In your case it should be the c# 11 project. Have a look at TestPositionalRecord1Async and update the failing ones the same way. Hopefully that will work.

SapiensAnatis · 2024-01-24T22:15:21Z

Ah, ok, thanks @bjornhellander. I wasn't sure if it was a case of needing to fix the analyzer or the tests, but it sounds like this is Roslyn behaviour so updating the tests is probably all that we can do. I will make those changes when I next take a look at this and hopefully have something ready for review.

SapiensAnatis · 2024-01-30T00:08:19Z

Regarding unifying ElementDocumentationBase.HandleDeclaration and the new code: I think it will be quite difficult to do unless SA1600 is adapted to inherit from ElementDocumentationBase, mainly because any changes to how HandleDeclaration parses documentation would require updates in quite a few analyzers. I wasn't sure whether adding that subclass would be advisable.

For now, I've added an extension method to XmlCommentHelper which returns a parsed XmlComment class that contains data from the XML comment. I was taking notes from the XmlFileHeader class here, but I like this as returning an object feels a bit less clunky than detecting inheritdoc tags using a out bool isInheritdoc parameter.

I also felt this was a bit better than pasting into a private method; a 'domain model' class to represent XML comments that are parsed either from the Roslyn syntax tree or from XElements from an included file feels like a nice abstraction, which could also be extended, should any other analyzers not inheriting from ElementDocumentationBase want to perform analysis of XML comments in the future.

Make SA1600 trigger on undocumented record primary ctor parameters

61b8a13

Add analysis of XML <param> tags to SA1600 so that it is raised on record primary constructor parameters.

bjornhellander reviewed Jan 23, 2024

View reviewed changes

SapiensAnatis added 2 commits January 29, 2024 23:15

Respond to review comments

703b1dd

Clean up tests

ad82a37

SapiensAnatis marked this pull request as ready for review January 29, 2024 23:57

SapiensAnatis changed the title ~~Draft: Make SA1600 trigger on undocumented record primary ctor parameters~~ Make SA1600 trigger on undocumented record primary ctor parameters Jan 29, 2024

Fix formatting

bcdc55b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SA1600 trigger on undocumented record primary ctor parameters #3783

Make SA1600 trigger on undocumented record primary ctor parameters #3783

SapiensAnatis commented Jan 22, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading

bjornhellander commented Jan 23, 2024

bjornhellander commented Jan 23, 2024

SapiensAnatis commented Jan 23, 2024

bjornhellander commented Jan 23, 2024

bjornhellander Jan 23, 2024

SapiensAnatis Jan 23, 2024 •

edited

Loading

bjornhellander Jan 24, 2024

SapiensAnatis Jan 29, 2024

bjornhellander Jan 23, 2024 •

edited

Loading

SapiensAnatis Jan 29, 2024

bjornhellander Jan 23, 2024

SapiensAnatis Jan 29, 2024

bjornhellander commented Jan 23, 2024

SapiensAnatis commented Jan 23, 2024

bjornhellander commented Jan 24, 2024 •

edited

Loading

SapiensAnatis commented Jan 24, 2024

SapiensAnatis commented Jan 30, 2024 •

edited

Loading

Make SA1600 trigger on undocumented record primary ctor parameters #3783

Are you sure you want to change the base?

Make SA1600 trigger on undocumented record primary ctor parameters #3783

Conversation

SapiensAnatis commented Jan 22, 2024 • edited Loading

codecov bot commented Jan 22, 2024 • edited Loading

Codecov Report

bjornhellander commented Jan 23, 2024

bjornhellander commented Jan 23, 2024

SapiensAnatis commented Jan 23, 2024

bjornhellander commented Jan 23, 2024

bjornhellander Jan 23, 2024

Choose a reason for hiding this comment

SapiensAnatis Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

bjornhellander Jan 24, 2024

Choose a reason for hiding this comment

SapiensAnatis Jan 29, 2024

Choose a reason for hiding this comment

bjornhellander Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

SapiensAnatis Jan 29, 2024

Choose a reason for hiding this comment

bjornhellander Jan 23, 2024

Choose a reason for hiding this comment

SapiensAnatis Jan 29, 2024

Choose a reason for hiding this comment

bjornhellander commented Jan 23, 2024

SapiensAnatis commented Jan 23, 2024

bjornhellander commented Jan 24, 2024 • edited Loading

SapiensAnatis commented Jan 24, 2024

SapiensAnatis commented Jan 30, 2024 • edited Loading

SapiensAnatis commented Jan 22, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading

SapiensAnatis Jan 23, 2024 •

edited

Loading

bjornhellander Jan 23, 2024 •

edited

Loading

bjornhellander commented Jan 24, 2024 •

edited

Loading

SapiensAnatis commented Jan 30, 2024 •

edited

Loading