dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.95k stars 4.02k forks source link

Tweak formatting for switch expressions #34272

Open jcouv opened 5 years ago

jcouv commented 5 years ago

Two questions:

  1. should we align the fat arrows? (illustrated below)
  2. where should the curly braces go?

Aligning fat arrows:

_ = e switch
{
    null => 1,
    _     => 2,
};

Current formatting:

_ = e switch
{
    null => 1,
    _ => 2,
};
CyrusNajmabadi commented 5 years ago

Feels like we might need an option here.

tmat commented 5 years ago

I'd say no aligning. We do not align case clauses of switch statement. Do we align anywhere else in a similar situation?

tmat commented 5 years ago

I'd say main reason not to align is that a single line change in one case may result in mutli-line diff.

CyrusNajmabadi commented 5 years ago

Do we align anywhere else in a similar situation?

The main problem is that we simply don't have any other similar situations :) We're in brand new territory :D

I'd say main reason not to align is that a single line change in one case may result in mutli-line diff.

A fair concern. Though one i would say is up to the individual dev/team to decide on. For example, the 'go' community pretty much mandates gofmt'ed code, and that aligns things all over the place. Turns out this is no biggie as you can trivially look at diffs in a whitespace insensitive manner.

I have a feeling (yes, it's just a gut feeling) that some reasonable subset of people will want alignment (and some will not). So my gut is coming down on this being an option.

That said, if we want to avoid an option, we should likely default to no-alignment.

sharwell commented 5 years ago

I find the current formatting most consistent with the rest of our formatting features. Vertical alignment should be covered by the feature request to support formatting extensions. I'm not sure what the request here is for brace placement, but I would expect it to match the style and options for other expressions with braces (object and collection initializers, lambdas, and anonymous functions).

tmat commented 5 years ago

Just curious, @jcouv why isn't the syntax using : instead of =>? Seems like it would be more consistent with ternary ?: operator.

_ = e switch
{
    null: 1,
    _: 2,
};
_ = e == null ? 1 : 2;
jcouv commented 5 years ago

@tmat Here are the notes I could find: https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-05-02.md#revisit-syntax-for-switch-expression In short, lots of debate, not many winning arguments.

@gafter those notes said the syntax is only tentatively decided for prototype. Do you recall if we discussed it again and decided to keep it, or is it rather a de facto decision at this point?

sharwell commented 5 years ago

Tentatively marking as By Design, but will update following design review.

sharwell commented 5 years ago

Design review conclusion:

  1. Update the formatter to ignore more than one space between the switch arm expression and => (allows for manual alignment of =>)
  2. The current brace placement is fine for now (anchor to first token on line containing the first token of the switch expression)
CyrusNajmabadi commented 5 years ago

Update the formatter to ignore more than one space between the switch arm expression and => (allows for manual alignment of =>)

Small suggested tweak: this makes sense in the absence of elastic trivia. in that case we're preserving intent. We probably want the 'hammer of thor' formatter, and the normal formatter to normalize on a single-space if there is elastic trivia.

ViIvanov commented 5 years ago

I have all "Place open brace on new line for…" except "…types" uncheked in Visual Studio options, but switch expression formatted as:

static int Main(string[] args) {
  switch(args) { // OK
  case string[] oo when oo.Length > 10:
    return 1;
  }

  return args switch
  { // Wrong, moved to a new line
    string[] ee when ee.Length > 5 => ee.Length,
    _ => -1,
  };

  // Expression above ("open brace") should formatted as:
  return args switch {
    string[] ee when ee.Length > 5 => ee.Length,
    _ => -1,
  };
}

Is it the same issue or it is better to make a separate one?