Simplify C# syntax generators. by CyrusNajmabadi · Pull Request #39077 · dotnet/roslyn

CyrusNajmabadi · 2019-10-04T21:18:16Z

It's probably a good idea to review this PR one commit at a time.
This PR should also be reviewed with whitespace changes OFF. Entire commits will become "only whitespace changed" doing that.

This PR simplifies the implementation of our syntax generator by using the existing (but uncommonly used) functionality to indent/dedent/block pieces of code. Instead, most of our writing code had to manually insert spaces, making it super annoying to keep track of current indent and emit the write amount of spaces.

This has been a pain every time i've edited SourceWriter so i thought i would finally clean it up. While i was in there, I also updated how we emit a few constructs to:

keep them formatted like we normally do in our codebase.
Use more modern constructs for brevity (i.e. =>, switch expressions conditional exprs, and the like).

CyrusNajmabadi · 2019-10-04T21:20:59Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/AbstractFileWriter.cs

Prevents people from calling WriteLine("") and having that actually emit pointless whitespace.

Since that file seems to be in a proprietary part, I couldn't check the context, but provided msg is a string, would perhaps string.IsNullOrEmpty be more prudent?

Since that file seems to be in a proprietary part,

I'm not sure what that means @tamlin-mike

would perhaps string.IsNullOrEmpty be more prudent?

Callers aren't passing in null here :)

I'm not sure what that means

@CyrusNajmabadi Clicking the file link resulted in "We went looking everywhere, but couldn’t find those commits". Searching for that filename in the roslyn repo didn't show anything. That suggested to me it's in a proprietary non-public part of the roslyn repository only Microsoft and its apointees have access to.

Callers aren't passing in null here :)

If that can be assured with 100% certainty, now and forever, OK.

Maybe make it a Debug.Assert( text != null ) ?

That suggested to me it's in a proprietary non-public part of the roslyn repository only Microsoft and its apointees have access to.

I'm not MS, or an appointee of it :-)

The file can't be found due to quirks around how GitHub works with force-pushes. If you just view the pr changes, you'll see this change

@AdamSpeight2008 I don't really see any value there. Thanks

CyrusNajmabadi · 2019-10-04T21:21:49Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

never used.

CyrusNajmabadi · 2019-10-04T21:22:09Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

Functions that already exist and already emit a { and update the indentation for what is written.

CyrusNajmabadi · 2019-10-04T21:22:19Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

this file doesn't use any usings.

CyrusNajmabadi · 2019-10-04T21:22:39Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

Many WriteLines were removed or moved to make newlines more consistent between constructs.

sharwell · 2019-10-04T21:23:11Z

@dotnet/roslyn-compiler Please consider reviewing and merging #37758 before merging this one.

CyrusNajmabadi · 2019-10-04T21:23:11Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

common cases were improved to produce less code.

CyrusNajmabadi · 2019-10-04T21:23:27Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

updated to use => bodies and switch expressions.

CyrusNajmabadi · 2019-10-04T21:24:14Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

both calls passed 'false' for the first argument. removed hte argument and simplified the impl since htis is always false.

CyrusNajmabadi · 2019-10-04T21:24:43Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

boilerplate code like this removed all blank lines between members. They didn't really help readability of the generated code.

CyrusNajmabadi · 2019-10-04T21:25:24Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

simple functional updates/withers/etc. just moved to inline their values instead of needing to explicitly make all the temporaries.

CyrusNajmabadi · 2019-10-04T21:26:13Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

constructs like array initializers can have commas after every item. So this just simplified things by always emitting the comma. We do the same thing for switch expressions when we use them.

CyrusNajmabadi · 2019-10-04T21:26:52Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

we were inconsistent between single-line if checks, or an if with an indented next statement. I tried to consistently use the same pattern everywhere.

Shouldn't the code produce be if ({0} is null) throw ?

There is no operator overloading with these types.

Doesn't @jaredpar say ... is null the definitive null check?

i don't see this discussion as being valuable for the core issues ths PR is attempting to solve. If you really want to make this change feel free to submit your own PR here.

CyrusNajmabadi · 2019-10-04T21:27:34Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

special cases switches where we only have a single case. In that case (no pun intended) we only use an 'if' check for consistency with the null checks.

CyrusNajmabadi · 2019-10-04T21:28:07Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

we're already in the Microsoft.CodeAnalysis.CSharp.Syntax namespace. So this could be abbreviated. This also makes us more consistent with the rest of the file (where we do this already).

CyrusNajmabadi · 2019-10-04T21:28:31Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

remove errant blank spaces at the end of lines. i.e. after {3} .

CyrusNajmabadi · 2019-10-04T21:29:22Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

tried to keep single-expr properties always as =>.

sharwell · 2019-10-04T21:29:38Z

📝 Commit 31e71f68dc29bb28e3704d46b1658e95702bda42 produces bad indentation which makes the commit impossible to review

CyrusNajmabadi · 2019-10-04T21:29:58Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

simplified cases where we generate a switch that only contained index switch { _ => null; } into just => null;

CyrusNajmabadi · 2019-10-04T21:30:27Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

simple forwarding methods (like Visits/Accepts/etc.) just moved to => methods.

CyrusNajmabadi · 2019-10-04T21:30:35Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

first arg an unused constant.

CyrusNajmabadi · 2019-10-04T21:31:04Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

could consider making this a single line. i don't care much.

sharwell · 2019-10-04T21:31:18Z

Commit 4dff45f94b2dc8ef56207b432d1cf91ed76e1bdf changes the generated code, but doesn't change the generator, indicating one of the reviews before or after it is not consistent.

CyrusNajmabadi · 2019-10-04T21:33:26Z

Tagging @dotnet/roslyn-compiler @RikkiGibson . It's such a pain having to work with SourceWriter. This is an attempt to make things more consistent and easy.

I have other things like i'd like to do in the future. For example, i think it's really unpleasant how we handle lists of things and trying to determine how to best separate lists of things with whitespace.

I think we could easily make some helpers here to make that easier and remove all the constant need for code to keep track of which list item it is on and so on. But i'd like to do that later.

sharwell · 2019-10-04T21:35:46Z

@CyrusNajmabadi It would be a huge help if you could work to be more consistent with synchronized commits. For example, if you look at #37758, you'll find that every commit in the pull request is consistent: the changes to the source generator always exactly match the changes in the generated code. If the commit didn't produce a reviewable (concise and understandable) change to the output, I updated the generator and code so it would be before trying to send it for review.

CyrusNajmabadi · 2019-10-04T21:57:45Z

@CyrusNajmabadi It would be a huge help if you could work to be more consistent with synchronized commits. For example, if you look at #37758, you'll find that every commit in the pull request is consistent: the changes to the source generator always exactly match the changes in the generated code. If the commit didn't produce a reviewable (concise and understandable) change to the output, I updated the generator and code so it would be before trying to send it for review.

I use VS as my git system. I simply tell it to commit. This inconsistency it can produce (where it seems to miss files) is something i've mentioned many times in the past. I do not know what to do about it, other than just avoid using it entirely and do thins from the command line. However, i was told to stop doing that and to use the built-in functionality because running from the command line could lead to significant perf issues in VS.

From what i can tell, possibly due to my underpowered system, the VS git system just misses things. Maybe it's because it's processing stuff in the background, but the UI doesn't wait until the bg work is done. I honestly have no idea waht to do about this other than comitting, then checking if the commit is correct at every step.

CyrusNajmabadi · 2019-10-04T22:02:14Z

Because of the size of this diff, i will attempt to re-apply all my changes and try to get the vs-git plugin to work properly there. it's not fun doing this as it means going and checking my disk (and github) are correct after each commit.

…iteLine

CyrusNajmabadi · 2019-10-07T21:52:12Z

@sharwell @333fred @jcouv @dotnet/roslyn-compiler . This is ready for review. As before, i recommend reviewing one commit at a time, and with whitespace changes off.

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

333fred · 2019-10-08T00:44:49Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

+                Write(
+                    $"public{(isNew ? " new " : " ")}{node.Name} With{StripPost(field.Name, "Opt")}({type} {CamelCase(field.Name)})" +
+                    " => Update(");


Consider putting this Write on one line.

333fred · 2019-10-08T00:50:25Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

            {
-                return string.Format("default({0})", GetRedPropertyType(field));
+                var type = GetRedPropertyType(field);
+                return type == "SyntaxTokenList" ? "default(SyntaxTokenList)" : "default";


I'm actually not a fan of this change. There are lots of lines there's just a bunch of typeless default literals, which is even less info than there is now.

Hrmm... i could return it. But i didn't think there was a lot of value in those 'defaults'.

An alternative would be using named parameters, which I would be fine with.

333fred · 2019-10-08T00:57:24Z

Got to f4f0bec.

333fred · 2019-10-08T18:35:35Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

-                            WriteLine("    ? new SyntaxToken(this, slot, {0}, {1})", GetChildPosition(i), GetChildIndex(i));
-                            WriteLine("    : default;");
-                            CloseBlock();
+                            WriteLine($"return slot != null ? new SyntaxToken(this, slot, {GetChildPosition(i)}, {GetChildIndex(i)}) : default;"); CloseBlock();


Possible that you took care of this in a later commit, but that CloseBlock(); needs to go on a new line.

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

333fred · 2019-10-08T19:20:38Z

@CyrusNajmabadi left comments I would like to see addressed unresolved.
Next time, smaller PR please :)

CyrusNajmabadi · 2019-10-08T19:24:26Z

@333fred to followup in next PR:

use interpolations consistently
Cleanup this line: Simplify C# syntax generators. #39077 (review)
Cleanup this line: Simplify C# syntax generators. #39077 (review)
Cleanup this line: Simplify C# syntax generators. #39077 (review)

Doe that work for you?

333fred

LGTM with followup PR to address formatting nits.

CyrusNajmabadi · 2019-10-08T19:38:49Z

@jcouv want to take a look?

CyrusNajmabadi · 2019-10-09T19:24:31Z

@dotnet/roslyn-compiler @sharwell can someone else take a look? Thanks!

RikkiGibson · 2019-10-09T19:44:41Z

Could we get a senior review for this please @dotnet/roslyn-compiler

CyrusNajmabadi · 2019-10-14T17:21:17Z

@jcouv Could you take a look? Thanks! :)

Note: i've heard that this PR isn't great for codeflow. HOwever, each commit is necessary here :-/

The PR isn't so bad on github.com, as long as you disable whitespace. Thanks!

jcouv · 2019-10-14T20:29:35Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

+                        if (IsOptional(field))
+                        {
+                            kinds.Add(new Kind { Name = "None" });
+                        }


nit: line break?
Hum, I guess it's okay. Many other instances in this file...

yeah. won't fix for now :)

jcouv · 2019-10-14T21:12:38Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

                        }
                        else
                        {
-                            WriteLine("      get {{ return new SyntaxToken(this, ((Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.{0})this.Green).{1}, {2}, {3}); }}", node.Name, CamelCase(field.Name), GetChildPosition(i), GetChildIndex(i));
+                            WriteLine(" => new SyntaxToken(this, ((Syntax.InternalSyntax.{0})this.Green).{1}, {2}, {3});", node.Name, CamelCase(field.Name), GetChildPosition(i), GetChildIndex(i));


nit: is this space before => intentional? (same question a few lines below)

yes, very much so. basically, we're emitting a property and we may emit it multiline or single line. so first we emit the header as something like public SyntaxToken Whatever (with no trailing spaces). Then the following code either appends => ... to it (with a leading space to get things looking good) or it appends a full accessor body with newlines and indentation.

Ah, got it. I thought we were using newline+indentation.

jcouv

LGTM Thanks (iteration 80)
I would squash this change though

jcouv · 2019-10-14T23:05:49Z

Thanks!

CyrusNajmabadi · 2019-10-14T23:33:32Z

Thank you!

CyrusNajmabadi requested a review from a team as a code owner October 4, 2019 21:18

CyrusNajmabadi commented Oct 4, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Outdated

Copy link

Contributor Author

CyrusNajmabadi Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never used.

CyrusNajmabadi commented Oct 4, 2019

View reviewed changes

CyrusNajmabadi added 2 commits October 4, 2019 15:06

Do not emit an indent if the caller only passed an empty string to Wr…

14126b7

…iteLine

Remove unnecessary usings from files.

c64f3ec

333fred self-assigned this Oct 7, 2019

333fred reviewed Oct 8, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

333fred reviewed Oct 8, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

333fred reviewed Oct 8, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

333fred reviewed Oct 8, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

333fred reviewed Oct 8, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

333fred reviewed Oct 8, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

333fred reviewed Oct 8, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

333fred approved these changes Oct 8, 2019

View reviewed changes

RikkiGibson removed their request for review October 8, 2019 21:58

jcouv self-assigned this Oct 10, 2019

jcouv reviewed Oct 14, 2019

View reviewed changes

jcouv approved these changes Oct 14, 2019

View reviewed changes

jcouv added this to the 16.4.P3 milestone Oct 14, 2019

jcouv merged commit 9e90377 into dotnet:master Oct 14, 2019

CyrusNajmabadi deleted the simplifyGeneratedCode branch October 15, 2019 00:24

Conversation

CyrusNajmabadi commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Oct 4, 2019

Uh oh!

CyrusNajmabadi commented Oct 4, 2019

Uh oh!

sharwell commented Oct 4, 2019

Uh oh!

CyrusNajmabadi commented Oct 4, 2019

Uh oh!

CyrusNajmabadi commented Oct 4, 2019

Uh oh!

CyrusNajmabadi commented Oct 7, 2019

Uh oh!

CyrusNajmabadi commented Oct 4, 2019 •

edited

Loading

CyrusNajmabadi commented Oct 8, 2019 •

edited

Loading