Naming Conventions parity between Roslyn and System.Reflection?

Question

It apparent that you guys have done a lot of work on Roslyn thus far and done some truly incredible design. Well done! I'm hoping to put Roslyn to some good use.

Having played with it a while I can't help but get this nagging feeling about some of the names of members you've chosen. "Opt" was getting on my nerves and I see you've decided to change it. Fantastic!

Here are some of my other peeves

Obviously the purpose of a project like Roslyn is different from Reflection, however I can't help but feel things rub me just slightly the wrong way in some cases. For example in a MethodDeclarationSyntax instance:

1. ReturnType.PlainName versus just Name in reflection.

2. ParameterList.Parameters[0].Identifier.Value. Would not "Name" be a better choice? I'm obviously looking at it from a "user's" perspective while I think your names are from the perspective you would see them. Because I don't see how identifier.Value would be the "name" :).

There are quite a few some things that I find. But rather than give you a whole list I just want to make sure that:

1. What I'm saying make sense

2. The perspective I looking at this from is the intended/correct perspective

---

http://www.matlus.com

Answer 1

Thanks for the feedback Shiv. I am not sure why we use ReturnType.PlainName as opposed to ReturnType.Name - but I think this may have something to do with the fact that unlike System.Reflection.Type.Name, PlainName does not include any details about the type's genericity. For example, the Reflection Name for IEnumerable<int> is "IEnumerable`1" whereas the PlainName for the same type is just "IEnumerable".

Regarding .Value - identifiers are represented using the type 'SyntaxToken' (which is used to represent every significant piece of text in the source code such as literals, punctuation etc.). SyntaxTokens have no concept of 'Name' and the only real piece of information they encapsulate is the 'Text' that they represents. SyntaxToken.GetText() returns this text exactly as it is present in the source code. SyntaxToken.Value returns the object 'value' of the text represented by the SyntaxToken. And SyntaxToken.ValueText returns the text representation of this value.

For example, say you have the following statement - 'long @long = 1L;'.

SyntaxToken.Value and SyntaxToken.ValueText for the escaped identifier '@long' both return the string "long". However, SyntaxToken.GetText() for the same identifier returns the string "@long".

Similarly, SyntaxToken.Value for the literal '1L' returns the boxed integer '1'. SyntaxToken.ValueText for the same literal returns the string "1". SyntaxToken.GetText() however returns the text exactly as it is present in the code i.e. the string "1L".

Here's some snapshots of the documentation comments for Value and ValueText that also talk about other types of SyntaxTokens like string literals, constants etc. -

---

Shyam Namboodiripad | Software Development Engineer in Test | Roslyn Compilers Team

Answer 2

Being that it's a C# API Name implies the C# mangling. Adding Plain begs the question, "Plain from what?" IMHO I'd expect Name to be C# mangled and MetadataName (if it's exposed) to be metadata mangled (eg `1). 

I agree in spirit with Shiv; Roslyn would benefit from a name scrub by the BCL team.

For example, could ChildNodesAndTokens be shortened to just Children? I know that doesn't imply the Trivia would be excluded -- but then again, it's triva and not really part of the "main" tree. They're just hanging off of the leaf nodes...

Roslyn could also use a convention scrub from the BCL team. CommonChildSyntaxList seems like an enumerator with an Indexer. Isn't there a BCLish way to expose that w/o having to create a new class?

More generally the enumeration abstractions could use a once over. ReadOnlyArray<T> doesn't implement IEnumerable<T>. Yikes. From the comment I'm guessing it's done to dodge boxing. But I thought the struct will only get boxed if it's cast to an IEnumerable<T>... If that's the case the optimized subset of LINQ operations would not get boxed and users would still get the full set of linq extensions.  

Thanks,
Chris 

Answer 3

Shiv and Chris - Thanks much for the valuable feedback! I would encourage you to log bugs on Connect for your suggestions above. Alternately, I can log bugs internally on your behalf for these suggestions.

---

Shyam Namboodiripad | Software Development Engineer in Test | Roslyn Compilers Team

Answer 4

Being that it's a C# API Name implies the C# mangling. Adding Plain begs the question, "Plain from what?" IMHO I'd expect Name to be C# mangled and MetadataName (if it's exposed) to be metadata mangled (eg `1). 

I agree in spirit with Shiv; Roslyn would benefit from a name scrub by the BCL team.

For example, could ChildNodesAndTokens be shortened to just Children? I know that doesn't imply the Trivia would be excluded -- but then again, it's triva and not really part of the "main" tree. They're just hanging off of the leaf nodes...

Roslyn could also use a convention scrub from the BCL team. CommonChildSyntaxList seems like an enumerator with an Indexer. Isn't there a BCLish way to expose that w/o having to create a new class?

More generally the enumeration abstractions could use a once over. ReadOnlyArray<T> doesn't implement IEnumerable<T>. Yikes. From the comment I'm guessing it's done to dodge boxing. But I thought the struct will only get boxed if it's cast to an IEnumerable<T>... If that's the case the optimized subset of LINQ operations would not get boxed and users would still get the full set of linq extensions.  

Thanks,
Chris 

There actually was just a Children property orginally. The feedback from usability testing showed that the #1 problem people had using the syntax API was using this collection. The names were changed to emphasize the distinction between nodes and tokens and to steer people away from falling into using the NodeOrToken type by default. So now there is also ChildNodes, DescendantNodes, etc.

As for ReadOnlyArray<T>.. Yes, it needs to be fixed.  We originally had it as IEnumerable, but removed it to force ourselves to not rely on it in core compilation scenarios. Excessive use of it was causing perf problems.

---

Wayward LINQ Lacky

Answer 5

Adding on to what my teammates have already offered here's some context on PlainName. The PlainName is more of a utility property than anything else. It's not related to name mangling or metadata names like in the Symbol API; it's purely syntactic. Here's a list of names that can be represented in the Roslyn Syntax API. I've highlighted the part that's returned by PlainName in the various overrides.

• System.Collections.Generic
• System.Collections.Generic.Dictionary<TKey, TValue>
• int
• List<int>
• DateTime
• byte*
• PropertyInfo[]
• Guid?
• V1::ClassLibrary1

Really the "plain name" is the simple atomic right-most quasi-colloquial name of a type or namespace or the element or underlying type of a pointer, array, or nullable type.

A bit of a fuzzy definition but it's used in a number of places in our code base. Unfortunately the language specification doesn't have a term to describe this thing and the spec does have meanings for the terms Name, SimpleName, and Identifier so we can't use any of those so we called it PlainName for now. It's definitely a property we're reviewing and it's status in future releases is subject to change.

 

Regards,

-ADG

---

Anthony D. Green | Program Manager | Visual Basic & C# Languages Team