Smart Indent does not handle fluent sequences correctly

[sharwell]

Version Used: Visual Studio 2019 Preview 2

🔗 Originally filed as https://devdiv.visualstudio.com/DevDiv/_workitems/edit/237805

Steps to Reproduce:

Press Enter at the location of the $$:

public class Test
{
    public void Test()
    {
        new List<DateTime>()
            .Where(d => d.Kind == DateTimeKind.Local ||
                        d.Kind == DateTimeKind.Utc)$$
            .ToArray();
    }
}

Expected Behavior:

public class Test
{
    public void Test()
    {
        new List<DateTime>()
            .Where(d => d.Kind == DateTimeKind.Local ||
                        d.Kind == DateTimeKind.Utc)
            $$
            .ToArray();
    }
}

Actual Behavior:

public class Test
{
    public void Test()
    {
        new List<DateTime>()
            .Where(d => d.Kind == DateTimeKind.Local ||
                        d.Kind == DateTimeKind.Utc)
                        $$
            .ToArray();
    }
}

[tiesmaster]

@sharwell I'd like to take a stab at this, though, it's going to be my first PR to Roslyn, so bear with me, please ;)

---

I've already reproduced the issue in an unit test, and tracked it down to CSharpIndentationService.Indenter. I see that

• GetDesiredIndentationWorker goes into GetIndentationBasedOnToken;
• and that the switch statement at the end of that method doesn't handle the CloseParenToken with a dedicated case, so it takes the default path using GetDefaultIndentationFromToken;
• this special cases query expressions, and delegates to GetDefaultIndentationFromTokenLine;
• this in turn detects that the containing syntax node spans multiple lines, and uses GetIndentationOfLine to get the indentation of the line with the closing parenthesis token, and uses that for the indentation.

Now, this can be handled at the last spot, just before calling GetIndentationOfLine, but I don't think that's the right location. This can also be handled as a special case, next to the "query expression" special case, but also doesn't really the right spot. Instead, I think it makes sense to add the closing parenthesis token as case to the switch, and detect a multi line ArgumentList, and use the indentation of the line of the first token of the argument list.

Am I going into the right direction with this. Any thoughts?

[tiesmaster]

@sharwell Maybe you missed my comment (☝️ ). Do you have any thoughts on this?

[sharwell]

I've already reproduced the issue in an unit test, and tracked it down to CSharpIndentationService.Indenter.

Nice!

Also, great documentation of the analysis work!

Instead, I think it makes sense to add the closing parenthesis token as case to the switch, and detect a multi line ArgumentList, and use the indentation of the line of the first token of the argument list.

Generally I agree. We need to make sure these two cases are handled though:

new Type().Method(d ||
                  c)

new Type()
    .Method(d ||
            c)

The second one matches the rule you described (use indentation of first token of argument list), but the first one actually needs to be indented one additional level relative to the indentation of the line where the argument list starts.

[tiesmaster]

Ah, nice. I was concerned about those scenario's (and I was tinkering about that), but this clears that. Just for completeness sake, it should be like this, right?

new List<DateTime>()
    .Where(d => d.Kind == DateTimeKind.Local ||
                d.Kind == DateTimeKind.Utc)
    $$
    .ToArray();

new List<DateTime>().Where(d => d.Kind == DateTimeKind.Local ||
                                d.Kind == DateTimeKind.Utc)
    $$

I have this working in a local branch, I'll clean up, and put that in a PR ;)

---

BTW Personally, when using fluent apis, I always put the first method chain on the next line, like the first example, because I know that the indenter, and formatter doesn't align on the "dot", like below. Are there any plans on supporting that?

new List<DateTime>().Where(d => d.Kind == DateTimeKind.Local || d.Kind == DateTimeKind.Utc)
                    $$
                    .ToArray();

[sharwell]

Are there any plans on supporting that?

We have plans to create an API where custom formatting rules can be added. I'm not happy with any proposals I've seen to handle alignment "in the box" because it has a bunch of unintended negative side effects on overall readability, especially when compared to the consistent behavior you get when you wrap before the first ..

[tiesmaster]

Fair enough, we used to use R# in my team, but I ditched that, because of the horrible formatting that came out of that, in conjunction with line wrapping. Usually, the lines tend to become really long, when you keep the . at the same line, so indeed, I also rather have the extra line, as opposed to a bunch of long lines, making it really unreadable.

Still, out of curiosity, are any of those proposals in public issues, or all internal? If public, do you have a couple links?

[sharwell]

@tiesmaster It's in #31691