Quick Fix: Add parameter to documentation

[rchande]

It'd be great if we had a quick fix for this warning:

[cmatskas]

@Pilchie @rchande I'll take this one :)

[rchande]

@cmatskas Awesome!

[cmatskas]

@rchande sorry for sounding silly but where in the code do you see this error message? the closest I can get to it is in SymbolDisplay.cs but there's no ISymbolDisplayCache param in any of the inline methods? Any pointers would be greatly appreciated :) thanks

[rchande]

@cmatskas I noticed it while adding the cache (and it hasn't been merged) 😄 .

There's a much shorter snippet where you can see it:

class Program
{
    /// <param name="args"></param>
    static void Main(string[] args, int args2)
    {

    }
}

[cmatskas]

Apologies folks but I may have to forgo this one. The solution crashes my VS2015 and all my attempts to even build it have failed. Back to the backlog as I cannot work on this :(

[jmarolf]

@cmatskas could you file a bug with the build errors so we can try and figure out why you can't build on 2015?

[cmatskas]

@jmarolf Sure, I'll open a new bug and attach a gist with the error messages. thanks

[jmarolf]

@cmatskas no, thank you!

[cmatskas]

Bug raised as #5656. I would be really keen to work with someone from the team to get this resolved and maybe contribute a fix or two (or more).

[cmatskas]

@rchande & @jmarolf now that I have a working solution I'm more than happy to work on this. How would you like this error message to be rectified? Remove it all together? Provide a prompt for adding the param tag in the xml comment? thanks for your help and feedback.

[rchande]

@cmatskas Glad you got your solution working! How much background do you have in Roslyn? I would write this as a CodeFixProvider(http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/CodeFixes/CodeFixProvider.cs,0ea452288308834d) that responds to that particular error ID and fixes the error by adding the missing parameter to the documentation. If you do this, when that error appears, you'll be able to type Ctrl-. and see the fix appear in the light bulb menu.

[janrapp]

I'm interested in contributing here. Is there still any interest in this?

[rchande]

@3jr definitely! We're still interested in this.

[janrapp]

I think it makes sense to apply this CodeFix to many items with a ParameterList and do something very similar for TypeParameterLists (a corresponding warning exists: CS1712).

VB doesn't have such a Warning, so no CodeFix for VB?

I have some issues with formatting (especially with /** */ comments). For /// comments the behavior I currently have implemented (see Current Behavior below) seems to work okay.

Current Behavior

(I'm totally willing to change this!)

1. Find the XML documentation of the previous parameter (*)
2. Insert \n/// <param name="{paramName}"><\param> after it
3. the Roslyn Formatter indents the line correctly

(*): The previous parameter in the method or indexer, etc. ParameterList.
If the previous parameter doesn't exists use an other previous parameter (the one closest to the one that we insert). If no previous parameter exist take the next following parameter and insert before. One other parameter must be documented otherwise these warnings doesn't apply. If no documented parameter is has a corresponding parameter insert before the first one (than the warning applies again).
If the previous parameter used ' (single quote) as quote character for the name attribute I also use it.

Issues with the current behavior

In the example

///    <param name="arg1"></param> <param name="arg2"></param> <param name="arg4"></param>
static void M1(int arg1, int arg2, int arg3, int arg4) { }

the current behavior leads to

///    <param name="arg1"></param> <param name="arg2"></param>
/// <param name="arg3"></param> <param name="arg4"></param>
static void M1(int arg1, int arg2, int arg3, int arg4) { }

potentially one could think of the following as issues

1. the indentation within the comment doesn't get preserved on the new line and
2. Initially the user probably wanted all parameter documentations on a single line and this behavior breaks this.

I think issue (1.) could be fixed but it's not obvious which indentation to use (and I couldn't yet figure out a way to compute one). For issue (2.) we would need to have some heuristic to figure out when and where to use \n.

I personally think we could live with those problems. Can anyone think of other issues?

Issues with /** */ comments

For /** */ comments the 3. step in the current behavior fails.

Just now I had the idea that one could use the indentation of the /** part to align the inserted documentation parameter. Given the current formatting rules this would result in something that is equivalent to the /// comments (unless there are non-whitespace chars in the line before the /**; then the whole comment doesn't get indented if formatting is applied).

Step 2. from above would be: insert "\n" + " " * c + " * <param name="{paramName}"><\param>" where c is the column of the /** of the comment.

I'm not sure I like that since maybe the user doesn't wan't to have " *" at the beginnings of comment lines; maybe they have everything alligned to the first column. Or want to take the whole content of the comment and process it as XML programmatically.

Maybe we could find a decent heuristic for that. I couldn't find a formatting rule in Visual Studio about comments. Is there another place where I could look for formatting rules but Options->Editor->C#->Formatting?

I could imaginge not supporting /** */ comments at all. At least I couln't think of a good way to format them.

Other Questions

Where to place this CodeFixProvider

Maybe a new folder in src\Features\CSharp\Portable\CodeFixes?

Name for the CodeFix

Maybe AddParameterXmlDocumentation. It's neither short nor unambiguously clear but I didn't find something I liked more yet.

Title that is shown to the user

Maybe: "Add param element in XML documentation comment for '{0}'" and "Add typeparam element in XML documentation comment for '{0}'" with param and typeparam locked for localization. (I probably could do a localization for German but that probably doesn't fit in the process)

"XML documentation comment" is a bit long but it seems to be the official name.

GetElementKind is internal

The compiler thinks in the following case that all parameter have documentation:

/// <summary> <param name="arg1"/></param> </summary>
/// <param name="arg2"></param>
/// <param name="arg3"/> 
static void M(int arg1, int arg2, int arg3 ) { }

I now look for the previous parameters correctly. If arg2 wouldn't be documented I would fix it to the following (not very pretty)

/// <summary> <param name="arg1"></param>
/// <param name="arg2"></param> </summary>
/// <param name="arg3"/> 
static void M(int arg1, int arg2, int arg3 ) { }

This is interesting in and of itself but to find all parameters correctly I search the comments just like the DocumentationCommentCompiler (we can discuss if this is too much code repetition in the pull request). This leads to me using the GetElementKind method in SyntaxExtensions which is internal to a different project. GetElementKind is the only method that has XmlNameAttributeElementKind in its signature and XmlNameAttributeElementKind is public. So that's to some extend inconsistent.

Edit: the (future) pull request may be a better place to discussed discuss the GetElementKind stuff entirely.

[rchande]

@3jr When you say you're inserting the string \n/// <param name="{paramName}"><\param>, do you mean you're modifying the Document by editing the text directly? You will probably have better luck with the formatter if you build up some DocumentationCommentTrivia nodes and insert them into the SyntaxTree next to the param tag you're targeting. (Let us know if you want more information on that.)

How do you handle ordering the tags? In your example above, if I document arg2 but not arg1 or arg3, do you show me fixes for both arg1 and arg3? Do they wind up in the parameter order regardless of the order I fix them in?

Your name for the fixer/location for the source seem fine.

For the string, maybe something like "Add XML documentation for parameter {0}"?

Other than those things, your approach sounds good. Let us know when you have a PR we can look at. Thanks!

[janrapp]

@rchande I don't edit the text directly. I believe I'm doing that correctly (At least it looks the same in the SyntaxTree). I was more describing how the end result looks like to the user.

The correct ordering of tags (no matter the order they are fixed in) is what I (hope to) get for inserting next to an other documentation tag I found. I believe (hope) the ordering isn't a problem with this approach.

What I have to look at again is how to fix all occurrences at once. Probably I look at this again. For some reason I don't get the behavior I expect, maybe there are merge conflicts or something.

I could use some information about roslyns unit test stuff. I don't really understand how the [| and {|named: spans are used. Is there documentation about that anywhere?
More specifically I would like to have some larger test case with many warnings -> have all of them fixed -> and assert the end result. I did build some small thing that does that for my initial project but that doesn't fit into roslyn. But then again I could just write many smaller unit tests.

[rchande]

@3jr Your sorting approach sounds okay. We'll be able to give you better feedback on that, and on the fix all occurences stuff, once you send out a PR.

Our markup strings let you mark up spans and named spans. This lets you specify things like a cursor selection, etc. When we have tests that need to look at multiple types of spans, we start naming them. Is there a specific test harness you want more information about?

The markup code itself lives here. There's some good documentation at the top of that file on how to use the markup.

If you have more questions about fix all, @mavasani is probably a good person to talk to.

[mavasani]

@3jr You can look at some sample FixAll unit tests here.

Note that the default BatchFixer does some very basic merge conflict resolution when you have conflicting fixes being batched - it behaves well when your individual code fixes are edits or removals, but has been found to have issues when they are adds - which seems to be the case here. You have couple of options:

1. Enhance/Fix the BatchFixAllProvider: Debug into this method which does the code fix merging and identify and possibly fix the unhandled cases.
2. Write your own FixAllProvider, you can take a look at a sample implementation here. You can probably reuse all the diagnostic computation part from there, and write your own code action that fixes all issues in the document.

[DustinCampbell]

Note -- this issue is essentially a dupe of #7070.

[Hosch250]

I am working on this as an extension of #7070. I hope to make a PR within a few days.

[janrapp]

@Hosch250 how far are you with this. I have some working code and I think I post it to #10520 (to see if the checks are successful). So you could use that as a starting point if you want to.

My solution only works for:

• C#
• /// comments
• Not as a Batch fix

I never meant to stall development for this; I apologize. I just started working on it because I thought I had a good idea how to tackle it. I was very unsure how to use the test facilities properly. Maybe I take a look at you PR and improve it a bit.

[Hosch250]

The only thing left is to make it place the parameter in the correct order in the list. I don't support VB.NET yet, but I designed with this in mind, so it will be extremely simple to add support for it. If you wish, I can help you learn how to do these correctly--I needed a lot of help the first time I did it too.

[Hosch250]

Also, if you decide to go through with it, you should definitely take a look at where I put my merged ones so everything is in the same folders.

[janrapp]

I'm looking forward to your implementation. I always wondered how I could implement this differently.

[Hosch250]

I'll tag you when I create the PR so you can help review it.

[Hosch250]

This issue was closed back in #12997

[Pilchie]

Oh, thanks for pointing that out!