Roslyn: Rename GreenNode to ParserNode

[jcouv]

The red/green distinction is confusing. The green/internal nodes can be derive from ParserNode (currently GreenNode).
Comments referring to "red nodes" should be updated to say "public nodes".

[sharwell]

💭 I never found this to be a particular barrier; I suspect anyone who needs to work at the layer of green nodes will be in the same boat. More concerning is the following:

1. This is a fairly substantial change, and the chance of someone hitting these types via reflection for something is fairly high.
2. I can't definitively say the new names are "better" for any definition of "better" that seems like an obvious win. Conceptually this compiler is fundamentally different from other compilers in this regard. Using special naming is therefore not a case of failing to use a well-known name for a well-known concept.

[jcouv]

@sharwell To your first point, we had a survey in the compiler team the other day during stand up and the confusion is apparent between red and green.

The proposal is to use "parser node" (green) vs "public node" (red) terminology. Alternative suggestions are welcome.

We only track public APIs as part of our contract and support commitment. So while people have been known to access internal APIs via Reflection, we should really consider their needs properly via public APIs. Internal APIs can always change.

[sharwell]

We had a survey in the compiler team the other day during stand up and the confusion is apparent between red and green.

Do we have any specific cases where this has caused difficulty for someone working in this layer? How will a naming change make it easier to understand the different semantics between the two layers?

Note that I'm not trying to say Green and Red are particularly helpful or descriptive. I am challenging the argument that the name of the base identifier is capable of having a meaningful impact on the ability of a developer to familiarize themselves with this section of the compiler.

Internal APIs can always change.

:memo: My suggestion otherwise is only valid in the context of failing to see a clear benefit from the change.

[jcouv]

I'm not worried about an individual working in the code. You look things up and then you're good to go.

The confusing names impair our ability to communicate clearly and discuss issues in this area as a team. Every such discussion so far was derailed in clarifying which is which, "but I thought it's the other way around", "here's my mnemonic, but I still usually get it wrong", etc. At some point, we need to fix the root cause.

[shaggygi]

@jcouv Not related to solution for what you are proposing, but I would like to eventually see an API that helps when dealing with parent/child similar to the red/green nodes.

See https://github.com/dotnet/corefx/issues/18357

I'm hoping Record Types (possibly coming in C# 8) will help this scenario.

BTW, the red/green distinction is confusing and the rename would probably help.

[AdamSpeight2008]

@jcouv I agree the names are confusing, especially as they don't reflect their usage.
We also have to consider "non-team" (potential) external contributors.

[CyrusNajmabadi]

1. It's an internal detail, so i would have no problem changing the internal names.
2. However, i don't find any of the names any better. If i say "ParserNode" and "PublicNode" there is nothing about that that explains what the difference is. I still have to teach someone "ParserNodes are different because they don't contain positions or parents. As such they can be shared, etc. etc. PublicNodes do have positions and parents. They are created and cached on demand as one walks a tree full of ParserNodes. They cannot be shared across trees, etc. etc"

So, overall, i don't really buy the motivation here. The current names aren't great, but i don't think changing them actually accomplishes the desired goal. I think we can accomplish this simply by putting documentation on the types and pointing people to it.

[alrz]

And at some point you "get used to it". I unconsciously read green as persistent internal etc. Changing it would take that away. If anything, I'd suggest "internal/public nodes" to at least cover a single aspect of it but as @CyrusNajmabadi said that still doesn't fully explain the underlying concept.

[jcouv]

As I pointed out above, I surveyed the compiler team in standup (everyone was there except Neal). That experience undeniably showed that we did not just "get used to it".
Everyone on the team knows the overall design (why we have two kinds of nodes), but almost everyone was unsure which, red or green, refers to which. They are just really bad names.

The desired goal is to align on unambiguous and memorable terms in discourse/culture.
Having names that describe the full design is not a requirement.
Enabling one person working in front of a code editor to "load his mental cache" by having great code comments solves a real but orthogonal problem (ie. not what I'm trying to solve).

We just need names with 1 bit of relevant information (as opposed to effectively zero or negative information as things stand) to facilitate communication.
"Parser node" or "internal node" both achieve that. "Green node" does not.

[tmat]

What about calling it InternalSyntaxNode?

[ArsenShnurkov]

PublicNode, GreenNode -> ImmutablePart

[alrz]

@ArsenShnurkov both are immutable types though.

[CyrusNajmabadi]

Also, we can't change the names of the public nodes (for obvious reasons). Only the internal impl details could have their names changes.

[ArsenShnurkov]

StablePart then