Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDE0025 strangely indents when a comment follows the property name #38057

Open
stephentoub opened this issue Aug 16, 2019 · 4 comments · May be fixed by #76582
Open

IDE0025 strangely indents when a comment follows the property name #38057

stephentoub opened this issue Aug 16, 2019 · 4 comments · May be fixed by #76582
Assignees
Labels
Area-IDE Bug Feature - IDE0025 Use expression body for properties help wanted The issue is "up for grabs" - add a comment if you are interested in working on it IDE-CodeStyle Built-in analyzers, fixes, and refactorings IDE-Formatter Code formatter and/or smart indent
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Aug 16, 2019

Version Used:
3.3.0-beta3-19413-06+ac06df1bffb7b7fd0e5bf63cc91921af76b21e03

Steps to Reproduce:

using System;

class Program
{
    static void Main() => Console.WriteLine(Foo);

    private static string Foo // cool prop
    {
        get { return "blah"; }
    }
}

Run IDE0025.

Expected Behavior:

using System;

class Program
{
    static void Main() => Console.WriteLine(Foo);

    private static string Foo => // cool prop
        "blah";
}

or

using System;

class Program
{
    static void Main() => Console.WriteLine(Foo);

    private static string Foo =>
         // cool prop
        "blah";
}

or

using System;

class Program
{
    static void Main() => Console.WriteLine(Foo);

    // cool prop
    private static string Foo => "blah";
}

Actual Behavior:

using System;

class Program
{
    static void Main() => Console.WriteLine(Foo);

    private static string Foo // cool prop
=> "blah";
}
@sharwell sharwell added Bug help wanted The issue is "up for grabs" - add a comment if you are interested in working on it IDE-CodeStyle Built-in analyzers, fixes, and refactorings labels Aug 17, 2019
@sharwell sharwell added this to the Backlog milestone Aug 17, 2019
@CyrusNajmabadi CyrusNajmabadi added the Feature - IDE0025 Use expression body for properties label Nov 1, 2022
@github-project-automation github-project-automation bot moved this to InQueue in Small Fixes Oct 22, 2024
@CyrusNajmabadi
Copy link
Member

This appears to be a formatting issue. We are property putting an elastic trivia at the start of the =>. the formatter is deciding it needs to put the => on the next line (because of the comment between it and the =>, but it's not properly indenting the token.

the formatting engine is so complex here, and i have no idea what portion is even responsible for handling this, or how it would need to be updated to take caer of this.

@heejaechang can you shed some light here?

@CyrusNajmabadi CyrusNajmabadi added the IDE-Formatter Code formatter and/or smart indent label Dec 8, 2024
@jnm2
Copy link
Contributor

jnm2 commented Dec 24, 2024

Investigating a little, there appear to be two hurdles.

  1. Elasticity is ignored when reformatting whitespace between non-whitespace trivia and a comment.
  2. The formatter doesn't apply additional wrapping indentation.

To demonstrate the latter point, with or without adding elastic whitespace before test, the following:

public class C
{
int // comment
test;
}

Formats as:

public class C
{
    int // comment
    test;
}

It does not apply further indentation to test the way it would wrap if you placed your cursor after // comment and pressed enter. This is something that could potentially be incorporated into the formatter.

The reason why an 'enter' keypress indents the way you expect, four spaces further in than the indentation level of the property itself, is that it falls through the big https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Indentation/CSharpIndentationService.Indenter.cs#L74 method, finds nothing special to do, and does the default indentation. The extra level of indentation is added at https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/BottomUpBaseIndentationFinder.cs#L149 and is determined by https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Indentation/CSharpIndentationService.Indenter.cs#L466.

So an enter keypress will add an additional increment of indentation by default. When it does not add this additional increment of indentation, such as after a semicolon or closing brace of a class member, this is because it no longer falls through the big indentation service syntax switch, and instead hits lines that override the int extraSpace parameter to 0. https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Indentation/CSharpIndentationService.Indenter.cs#L238-L261

@jnm2
Copy link
Contributor

jnm2 commented Dec 28, 2024

My proof of concept worked! It's one approach. It adds more indentation operations within single-line constructs which represent the wrapping that they should have if they were to break across multiple lines. main...jnm2:38057_wrapping_indentation_rules

Image

@jnm2
Copy link
Contributor

jnm2 commented Dec 30, 2024

Unfortunately, following this up is a lot of work. We wouldn't want to just capture this "extra level of indentation" choice when formatting elastic trivia, we'd want to follow all the logic in https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Indentation/CSharpIndentationService.Indenter.cs which special-cases dozens of scenarios to indent properly when you press Enter.

That logic itself calls into the formatter to make decisions. So the formatter can't easily call into this indentation-on-new-line logic.

Longterm, perhaps all this logic could be reimplemented by adding IndentBlockOperations computed by the formatting rules, making the formatting rules the source of truth. This would position the formatting rules to be able to codify things like how to wrap long lines, which is a community request: #15406

In this approach, the logic would change form. Instead of introspecting and deciding on the fly, spans would be computed up front along with the other indentation blocks. These spans would start one character in from the start of a syntactic construct, because pressing Enter there should indent on the next line, and likewise end one character in from the end of the construct. When braces and brackets and embedded statements are introduced, they will have to mesh well. I started experimenting with this and it seems like a great direction to go in, but it isn't a small investment.

In the meantime, fixing this bug probably requires the code fix to manually compute the indentation for the added => token, perhaps transferring it from the get token.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Bug Feature - IDE0025 Use expression body for properties help wanted The issue is "up for grabs" - add a comment if you are interested in working on it IDE-CodeStyle Built-in analyzers, fixes, and refactorings IDE-Formatter Code formatter and/or smart indent
Projects
Status: InQueue
Development

Successfully merging a pull request may close this issue.

6 participants