Don't gather all trivia of a parameterized property in the first accessor

[TymurGubayev]

Problem

#1095: VB -> C#: comments/trivia in wrong location (for parameterized properties)

Solution

• Introduce special handling of property blocks
• Which part of this PR is most in need of attention/improvement?
  This is current conversion result:

internal partial class IndexedPropertyWithTrivia
{
    // a
    // b
    public int get_P(int i)
    {
        // 1
        int x = 1; // 2
        return default;
        // 3
    }
    // c
    public void set_P(int i, int value)
    {
        // 4
        int x = 1; // 5
                   // 6
        x = value + i; // 7
                       // 8
                       // d
    }
}

Issues with the result:

1. the // d is before // c, instead of being attached to the closing bracket of the setter. this one is a relatively important problem. solved
2. the // b should be just before the opening bracket of the getter. This one would be nice to have.
3. the // d comment is moved inside the setter, which might be a problem if it's a compiler directive instead.

• At least one test covering the code changed

[TymurGubayev]

@GrahamTheCoder Currently I have no idea how to solve the still open issues without littering the code with lots of logic around trivia. Could you take a look?

[GrahamTheCoder]

I'll try to take a look today

[GrahamTheCoder]

I think what might make sense to debug these issues is to create a little helper we can enable, which outputs the original source but with tokens <1>, <2>, <3> representing each source mapping point and then the same for the output (with care that the tokens are numbered to match up with each other). Then it should be easy to see where an appropriate mapping point is wrong or needs to be added.

Of course there's still the difficulty of finding the most elegant place to put that code. I'd need to look back at the algorithm, but it may be for this case rather than removing a source mapping when a complex transformation happens, we perhaps could annotate it explicitly to say it's moved, and treat that specially. There will always be awkward edge cases (e.g. depending whether someone writes a comment referring to stuff above or below) but we can likely get improve things still

[TymurGubayev]

Basic problem here is, when CommentConvertingVisitorWrapper.ConvertHandledAsync is handling the VB.PropertyBlockSyntax, the converted = _wrappedVisitor.Visit(vbNode) is only the getter, which causes trivia attached to End Property to get mapped onto the closing brace of the getter.

Currently I only see adding special handling for this case as a solution.

[TymurGubayev]

@GrahamTheCoder I've solved the major issue, but two relatively minor still exist - see updated PR description. The No. 3 is somewhat more important, but both can be left alone for now, I think.

As a side-effect of the implementation, there is no new line between getter and setter methods (no idea why, though). I hope it's ok.

I thought about following alternative way to solve this:

Instead of returning the getter as the node for the property block, and the setter in _additionalDeclarations, we could return something like this:

void dummyP()
{
    public int get_P(int i)
    {
    }
    public int set_P(int i, int value)
    {
    }
}

i.e. getter and setter methods inside a dummy method as local functions (I think the syntax itself is valid, even if visibility modifiers aren't allowed).
This way, that dummy function can get the trivia from the property block the usual way.
The problem is of course when/how to remove it while keeping the insides and the trivia.

[GrahamTheCoder]

This looks good given the constraints - nice that it's in one block and seems in the right place.

I'm not that keen on trying to make an extra fake method wrapper. Some of the code already checks the shape of what is returned, then mutates it. I also try to avoid that pattern, but given it happens, mixing that with this suggestion seems slightly riskier.

Anyway, let's stick with this, thanks!

[TymurGubayev]

An alternative would be to mark the "deleted" source lines so that all their trivia is considered leading/trailing.
Or maybe just transform the source tree accordingly (move leading trivia from End Property into trailing trivia of End Set), before the semantic model (of the source) is created.

[GrahamTheCoder]

Yeah I think the first idea makes sense.

The underlying algorithm already tries to ensure no comments are lost from deleted lines but allows whitespace to be coalesced. I assume it's being coalesced in a different location.
Of course we could just make it so there's always a trailing newline on end braces of declararions or some such. I don't think it's very important to maintain exact spacing between members, just nice if it looks nice.

With mutatung the source tree, it is done for non-local changes like renames, but best to keep to a minimum since it generally makes it harder to debug/fix when you need to check more places, and decide which one to put the fix in.