Roslynator fix doesn't fix all instances of long line diagnostic (RCS0056)

[etiago]

Product and Version Used: Roslynator CLI 0.6.1.0 with Roslynator Analyzer 4.4.0

Steps to Reproduce:

Add the following to .editorconfig:

dotnet_diagnostic.rcs0056.severity = error
roslynator_max_line_length = 120

Run roslynator fix against a large project, with >1000 instances of lines longer than 120.

Actual Behavior:

When doing this, Roslynator will fix a bunch of lines in a bunch of files (around 500), but leaves many hundred behind.

When I run roslynator analyze, it clearly finds the ones that still have long lines, but refuses to fix them.

In fact, if I run roslynator fix again, it also finds them but leaves them behind under Unfixed diagnostics. It's unclear why these are unfixed... and I'd really like it for it to pick up all the ones it needs fixing, as fixing them all by hand will be painful.

[etiago]

If I run it multiple times, it does catch a few more every time, but eventually refuses to catch any more even though it does find them:

[josefpihrt]

Hi,

Thanks for the feedback 👍.

Fixer for this diagnostic needs to be improved. The reason why it does not fix all lines is that it's not trivial to determine how to wrap a line. And when I'm talking about wrapping a line I mean to format it so it's not too long but it looks nice and it's readable (not just inserting a newline at nearest location to make it short enough).

Could you send a link to a repo so it's possible to reproduce it?

Code fix provider:
https://github.com/JosefPihrt/Roslynator/blob/main/src/Formatting.Analyzers.CodeFixes/CSharp/LineIsTooLong/LineIsTooLongCodeFixProvider.cs

[etiago]

Ah I suspected this 😃 some of the instances that are left behind are long strings, which as you say, are difficult to wrap sensibly as you wouldn't want to e.g., cut a word in half.

Hmm... I'll probably need to do some manual hacking then :) unfortunately this is an internal repo so can't really share a link.

Thanks for this tool though, it's really useful!

[josefpihrt]

I'm glad you like Roslynator!

Maybe you can send just some lines which were not wrapped but look like they could be wrapped (not the long literals). Then possibly the fixer could be improved for these cases.

[etiago]

For some of them it feels like it should be possible to have a sensible wrapping.

For example, this one:

                                    bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa = bbbbbbbbbbbbbbbbbbbbbbbbbbb.cccccccccccc(dddddddddddddd, eeeeeeeeeeeeeeee.ffffffff, xxxxx.yyyyy);

It has quite a bit of space for indentation, but it should be possible to wrap after the equals (I'm using 120 as the limit), and then wrap on every argument to the method.

I have a similar example but with a function signature. I can manually wrap it by just dropping every argument into their own line, which I think is uncontroversial, but it's not doing it automatically at the moment.

[josefpihrt]

Yeah, this case should be fixable.

[etiago]

Fair :) if I have some time tonight I'll try to reproduce this in a public repo and see if I can help with producing a fix.

[etiago]

Just to add a bit more data to this, one of them is fairly easy to replicate.

Example with a concrete bit of code from one of my pet projects:

        public ActivePlayer? GetPlayerInGame(
            string username,
            string gameSessionId,
            string hubConnectionId)
        {
                                                           using var context = new GameDbContext();
            return context.ActivePlayers.FirstOrDefault(p => p.PlayerName == username && p.CurrentGame.Id == gameSessionId && p.ConnectionId == hubConnectionId);
        }

I added a bunch of whitespace before the using var context... and Roslynator detects it, but is unable to fix it.

Will see if I can figure out how to have a go at fixing this...

[etiago]

I'm a bit confused, as this seems like it should be working... There's a passing test that validates this...

[josefpihrt]

The easiest way to find out is to add test with your code snippet and see the result.

[etiago]

Yeah, I think this reproduces it:

public async Task Test_Assignment()
    {
        await VerifyDiagnosticAndFixAsync(@"
class C
{
    static string foo()
    {
[|                                                                                                              string x = foo();|]

        return null;
    }
}
",
@"
class C
{
    static string foo()
    {
                                                                                                                string x
                                                                                                                    = foo();

        return null;
    }
}
");
    }

This is with the original 125 max line length. 125 is right after the f so it should be possible to break after the equals.

But I get the following output:

No code fix has been registered.

Candidate actions:
-

Expected: True
Actual:   False

[etiago]

I think I'm getting closer to the root cause... What I found so far is the following.

Here Span.End starts at 46. I don't know where 46 comes from.

I traversed the node tree manually under the debugger, and I came across this node:

So the parser does seem to think that something starts at 46... but even thinking about different ways that this is being calculated, both including and excluding line breaks, etc... 46 always lands me somewhere in the middle of white space here:

Ultimately this makes us fall through here:

I suspect that the root cause is that the 46 is wrong. That span should probably be covering just the assignment, and excluding any whitespace... but maybe I'm wrong.

[josefpihrt]

The problem is with the following line:

int indentation = SyntaxTriviaAnalysis.GetIncreasedIndentationLength(node);

where indentation represents the length of the indentation of the wrapped (next) line. But because the line string x - foo(); is indented so much the indentation is calculated incorrectly.

I made the PR which should solve this issue.

[josefpihrt]

But this issue with the indentation shouldn't have anything to do with the code fixer for RCS0056 (on condition that the indentation is increased/decreased by single step, which is common case).

[etiago]

So I don't know... but using your PR branch, and re-running my test above, I can get it to pass by tweaking the indentation a bit.

[josefpihrt]

Your code sample is part of the PR and it's passing:

https://github.com/JosefPihrt/Roslynator/pull/1188/files#diff-f4557e260d2206259a61033fcdb42fb586be2761a75ab6aadb1a2d5c4863ee0cR702-R728

[josefpihrt]

PR is complete. Please make any further development on the PR branch before it's merged.

#1188

[josefpihrt]

merged #1188