Fix #29390, add space after tuple type and identifier by werwolfby · Pull Request #29405 · dotnet/roslyn

werwolfby · 2018-08-20T22:00:54Z

But I don't know what to do with this:

var a = new(string a, int b)[10];

From my point of view it should have space:

var a = new (string a, int b)[10];

Completely similar to:

var a = new string[10];
var b = new Lazy<int>[10];
// etc.

We don't have spaces only in 2 cases:

class Foo<T>
    where T : new() // case #1
{
    public Foo()
    {
        var a = new[2] { 1, 2 }; // case #2
    }
}

In both cases we don't specify any type here, so skip space looks natural.
But for tuple from my point of view it is unnatural, because tuple - is just a type, and for types we have extract space.

I've found a few discussions:
from @jcouv: dotnet/csharplang#100 (comment)
and @sharwell : #23686 (comment)

But I can't find final decision

Right now I'll leave test code commented. And will not add whitespace before tuple in new expression. I need your input on this.

CyrusNajmabadi · 2018-08-20T22:03:38Z

From my point of view it should have space:

I agree. it should have a space. When there's a type, it's new<space>type.

CyrusNajmabadi · 2018-08-20T22:05:54Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNormalizer.cs

i don't understand what the IsWord check is for.

you don't need ?. after token.Parent. All tokens you could reach in SyntaxNormalizer have parents.

I'd much rather we enumerate the cases where a tuple type would want a space after it (or where it woudl not want a space after it). Looking specifically for isword just seems busted.

In cases like:

(string a, (string b, string c))

I don't want to add space after c) so I want to have space only after )<identifier,word> the same expression exist above but for braket: https://github.com/dotnet/roslyn/blob/ec99ff789c32a34fe444f8631e5a5166d2ebc11d/src/Compilers/CSharp/Portable/Syntax/SyntaxNormalizer.cs#L400

So I think we want space after tuple only when next token is word.

…...)[..]

werwolfby · 2018-08-21T18:31:26Z

Add space after new and before tuple:

var a = new(string a, int b)[10];

Will be normalized to:

var a = new (string a, int b)[10];

werwolfby · 2018-08-21T18:35:12Z

@jcouv Can we point it to v15.9 (or earlier) milestone?

CyrusNajmabadi · 2018-08-21T18:51:28Z

I'm not sure this would meet any 15.9 bar...

werwolfby · 2018-08-21T19:12:54Z

From my point of view, it is too small changes to postpone it.

But I'm ok with it.

CyrusNajmabadi · 2018-08-21T19:19:03Z

From my point of view, it is too small changes to postpone it.

It's not postponed. It's just going into the next appropriate release. Right now 16.0 is where all current work is done unless it is critical for 15.9. A tiny formatting fix is not critical under any sort of assessment, and would thus be innapropriate IMO to take for 15.9.

werwolfby · 2018-08-21T19:21:30Z

@CyrusNajmabadi got it, thank you, I thought that 15.9 will be next release, not 16.

werwolfby · 2018-08-21T19:23:30Z

P.S. By the way I can't understand your branching strategy: dev15.9.x, dev15.9.x-vs-deps
What all this staff mean, I can't find any documentation on it

CyrusNajmabadi · 2018-08-21T19:33:55Z

P.S. By the way I can't understand your branching strategy: dev15.9.x,

'dev15.9.x' is the branch going into VS15.9. It contains all the changes roslyn wants to go in that release, and should only be taking PRs that have been specifically approved for that release based on a high 'bar' check that warrants it.

'master' is where pretty much all other work goes. At some point master will branch off onto whatever come after 'dev15.9.x'. That may be 'dev15.10.x', 'dev16.x', or something else entirely. At that point, that branch will again only be for approved changes for that specific release, and 'master' will again be for the release after that.

And so on and so forth. Effectively: we do work in master. And, from time to time, branches are made to capture the point in time containing all the work we want to go in a specific VS release.

jcouv

LGTM Thanks (iteration 2)

jcouv · 2018-09-28T19:56:31Z

@ivanbasov for second review. Thanks

jcouv · 2018-09-28T22:36:39Z

Closed/re-opened to kick CI.

jcouv · 2018-09-29T22:40:55Z

Merged. Thanks @werwolfby !

Since updating Microsoft.CodeAnalysis to 3.3.1, the automatic formatting for tuples in C# has been fixed, so update tests accordingly. dotnet/roslyn#29405

* Update to Microsoft.CodeAnalysis 3.3.1 This updates to the latest Microsoft.CodeAnalysis packages for C#8 support. Fixes the following error on a project that uses nullable reference types: Microsoft.DocAsCode.Exceptions.DocfxException: Unable to generate spec reference for !: ---> System.IO.InvalidDataException: Fail to parse id for symbol in namespace . at Microsoft.DocAsCode.Metadata.ManagedReference.YamlModelGenerator.AddSpecReference (Microsoft.CodeAnalysis.ISymbol symbol, System.Collections.Generic.IReadOnlyList`1[T] typeGenericParameters, System.Collections.Generic.IReadOnlyList`1[T] methodGenericParameters, System.Collections.Generic.Dictionary`2[TKey,TValue] references, Microsoft.DocAsCode.Metadata.ManagedReference.SymbolVisitorAdapter adapter) [0x0005e] in <27b606b838ab45b788854d5e45efa610>:0 at Microsoft.DocAsCode.Metadata.ManagedReference.SymbolVisitorAdapter.AddSpecReference (Microsoft.CodeAnalysis.ISymbol symbol, System.Collections.Generic.IReadOnlyList`1[T] typeGenericParameters, System.Collections.Generic.IReadOnlyList`1[T] methodGenericParameters) [0x00000] in <27b606b838ab45b788854d5e45efa610>:0 * Fix TODO for space after tuple Since updating Microsoft.CodeAnalysis to 3.3.1, the automatic formatting for tuples in C# has been fixed, so update tests accordingly. dotnet/roslyn#29405 * Fix TestCSharpFeature_Default_7_1Class Since updating to Microsoft.CodeAnalysis 3.3.1, the default keyword is resolved to a vaule instead of the a default expression. Probably comes from dotnet/roslyn#37596

werwolfby requested a review from a team as a code owner August 20, 2018 22:00

jaredpar added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 20, 2018

jcouv added this to the 16.0 milestone Aug 20, 2018

jcouv self-assigned this Aug 20, 2018

CyrusNajmabadi reviewed Aug 20, 2018

View reviewed changes

jinujoseph assigned ivanbasov Aug 21, 2018

werwolfby added 2 commits August 21, 2018 21:28

Fix dotnet#29390, add space after tuple type and identifier

c43c154

Add space before tuple in new tuple array expression: new (T a, T b, …

014820a

…...)[..]

werwolfby force-pushed the feature/fix-normalize-whitespaces-for-tuples branch from ec99ff7 to 014820a Compare August 21, 2018 18:29

werwolfby changed the title ~~WIP: Fix #29390, add space after tuple type and identifier~~ Fix #29390, add space after tuple type and identifier Aug 21, 2018

jcouv approved these changes Sep 28, 2018

View reviewed changes

ivanbasov approved these changes Sep 28, 2018

View reviewed changes

jcouv closed this Sep 28, 2018

jcouv reopened this Sep 28, 2018

jcouv merged commit 6dc2858 into dotnet:master Sep 29, 2018

werwolfby deleted the feature/fix-normalize-whitespaces-for-tuples branch September 29, 2018 23:30

dlech added a commit to dlech/docfx that referenced this pull request Oct 12, 2019

Fix TODO for space after tuple

cfe3cf1

Since updating Microsoft.CodeAnalysis to 3.3.1, the automatic formatting for tuples in C# has been fixed, so update tests accordingly. dotnet/roslyn#29405

Conversation

werwolfby commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Aug 20, 2018

Uh oh!

CyrusNajmabadi Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

werwolfby Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

werwolfby Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

werwolfby commented Aug 21, 2018

Uh oh!

werwolfby commented Aug 21, 2018

Uh oh!

CyrusNajmabadi commented Aug 21, 2018

Uh oh!

werwolfby commented Aug 21, 2018

Uh oh!

CyrusNajmabadi commented Aug 21, 2018

Uh oh!

werwolfby commented Aug 21, 2018

Uh oh!

werwolfby commented Aug 21, 2018

Uh oh!

CyrusNajmabadi commented Aug 21, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Sep 28, 2018

Uh oh!

jcouv commented Sep 28, 2018

Uh oh!

jcouv commented Sep 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

werwolfby commented Aug 20, 2018 •

edited

Loading