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

IDE0055 doesn't provide any control for semicolon positioning on while/for statements #38242

Open
stephentoub opened this issue Aug 23, 2019 · 6 comments
Assignees
Labels
Area-IDE Bug Feature - IDE0055 help wanted The issue is "up for grabs" - add a comment if you are interested in working on it IDE-Formatter Code formatter and/or smart indent
Milestone

Comments

@stephentoub
Copy link
Member

Version Used:
3.3.0-beta3-19415-01+f5ba9f2c61a2fe853dc4913888d40df221539147

Steps to Reproduce:

using System;

class Program
{
    static void Main()
    {
        Console.WriteLine("starting");
        while (!Ready);
        Console.WriteLine("done");
    }

    private static bool Ready => true;
}

Expected Behavior:
I should be able to configure my settings such that this code isn't changed by IDE0055.

Actual Behavior:
IDE0055 (formatting fixes in general) forces this to:

using System;

class Program
{
    static void Main()
    {
        Console.WriteLine("starting");
        while (!Ready) ;
        Console.WriteLine("done");
    }

    private static bool Ready => true;
}

which just looks wrong compared to the positioning of the semicolons in the other statements.

@sharwell
Copy link
Member

This seems like a bug in the current formatter handling of semicolons, but we'll go over it in design review to confirm.

@sharwell sharwell added Area-IDE Bug IDE-Formatter Code formatter and/or smart indent Need Design Review The end user experience design needs to be reviewed and approved. labels Aug 23, 2019
@sharwell sharwell added this to In Queue in IDE: Design review via automation Aug 23, 2019
@CyrusNajmabadi
Copy link
Member

IIRC, this was intentional (though the decision was made like 10+ years ago). the idea was "we should call attention here to make it clear that this is an empty-while". if the ; abuts the ) it's easier ot miss and users may think the next line is what is being looped.

That said, nothing is sacrosanct. We could change our defaults here. Or create a set of toub_formatting rules :)

@stephentoub
Copy link
Member Author

stephentoub commented Aug 23, 2019

Or create a set of toub_formatting rules :)

If this ends up being closed as by-design, let's absolutely do that! Long live toub_formatting rules. 😉

@CyrusNajmabadi
Copy link
Member

Note: we do have an open goal of allowing extensible formatting. That way, at the end of the day, if there is something very particular for your group, you can express it. That said, for at least some of your issues (like aligning variable decls), i think those are so common and widespread they should be considered as options.

@stephentoub
Copy link
Member Author

Also, I should note that in general I'm trying to respect the style of the codebase that's already there. In this case, a quick search across corefx shows ~250 uses of for (...); and ~15 uses of for (...) ; (I'm guessing the latter being cases where the IDE asserted its will 😉).

@vatsalyaagrawal vatsalyaagrawal added this to the 16.4 milestone Sep 4, 2019
@sharwell sharwell moved this from In Queue to Next meeting in IDE: Design review Oct 14, 2019
@sharwell
Copy link
Member

sharwell commented Oct 24, 2019

Design review conclusion:

  • The design may have been on purpose, with a goal of highlighting potentially-unexpected empty control flow blocks

  • We should create a new analyzer for the problematic cases instead of relying on the formatter

    1. Report a diagnostic if an indented statement follows a looping construct with an empty child statement, such as:

      if (condition);
        statement;

      ➡️ This is already covered by the formatting analyzer.

    2. Report a diagnostic if a block follows a looping construct with an empty child statement, such as:

      if (condition);
      {
      }

      ➡️ This will need a new analyzer.

  • We should update the formatter to no longer place the space between ) and ;. Existing code will be updated to the new style the next time Format Document is run. This is a rare case, so we are not expecting significant push-back for the formatting change.

@sharwell sharwell added help wanted The issue is "up for grabs" - add a comment if you are interested in working on it and removed Need Design Review The end user experience design needs to be reviewed and approved. labels Oct 24, 2019
@sharwell sharwell moved this from Next meeting to Complete in IDE: Design review Oct 24, 2019
@sharwell sharwell added this to InQueue in Small Fixes via automation Oct 24, 2019
@jinujoseph jinujoseph modified the milestones: 16.4, Backlog Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Bug Feature - IDE0055 help wanted The issue is "up for grabs" - add a comment if you are interested in working on it IDE-Formatter Code formatter and/or smart indent
Projects
Status: Complete
Small Fixes
  
InQueue
IDE: Design review
  
Complete
Development

No branches or pull requests

5 participants