C#: Teach unification library about nested types#3351
Conversation
80495ca to
88f2103
Compare
88f2103 to
84e9234
Compare
84e9234 to
0466e36
Compare
calumgrant
left a comment
There was a problem hiding this comment.
The overall idea looks very good - a few quibbles about GenericType that's all.
| module Gvn { | ||
| /** Gets the qualified name of type `t`. */ | ||
| string getQualifiedName(Type t) { | ||
| if not t instanceof NestedType or t.(NestedType).getDeclaringType() instanceof GenericType |
There was a problem hiding this comment.
I'm not sure I understand or t.(NestedType).getDeclaringType() instanceof GenericType. Why remove the qualifier of types such as Foo<>.Bar?
There was a problem hiding this comment.
getQualifiedName is used in two places, GenericType::toStringExt and ConstructedGvnTypeList::toStringConstructed; in the former, the generic qualifier is added without printed type arguments/parameters, and in the latter the generic qualifier is added including type arguments/parameters. This is the reason why the qualifier is not added here.
There was a problem hiding this comment.
Yes, the output is clearly correct, so there's no bug. It's just that this isn't the "qualified" name - it is something else. Perhaps update the name and comment to make this clearer - something like getNonGenericSuffix?
| * A generic type. This is either a type with a type parameter, a type with | ||
| * a type argument, or a nested type with a generic enclosing type. | ||
| */ | ||
| class GenericType extends Type { |
There was a problem hiding this comment.
I'm struggling a bit with this name, because it matches GenericType elsewhere. How about ParameterizedType, where it is understood that a type parameters include the declaring types? This could be moved into a separate file as it's not really specific to Gvn.
There was a problem hiding this comment.
The reason I went with GenericType is because it covers both parameterized types and constructed types. We also use that name with the same meaning elsewhere.
There was a problem hiding this comment.
The difference is that it also includes nested types that are only indirectly generic. Well, I don't want to quibble too much over a name, it was just a suggestion.
| } | ||
|
|
||
| /** Gets the `i`th child of this type, taking nested types into account. */ | ||
| Type getChildExt(int i) { |
There was a problem hiding this comment.
How about getParameter(int i). Ext could be more descriptive.
There was a problem hiding this comment.
Again, this is not restricted to parameterized types, but also covers constructed types. But I agree that Ext is very generic (no pun intended).
| int getNumberOfChildrenSelf() { result = count(int i | exists(this.getChild(i)) and i >= 0) } | ||
|
|
||
| /** Gets the number of children of this type, taking nested types into account. */ | ||
| int getNumberOfChildrenExt() { |
There was a problem hiding this comment.
getNumberOfParameters() ? - Where it is understood that a parameter of a parameterized type spans all declaring types.
There was a problem hiding this comment.
Same comment as above.
| } | ||
|
|
||
| /** Gets the generic containing type, if any. */ | ||
| GenericType getQualifier() { result = this.(NestedType).getDeclaringType() } |
There was a problem hiding this comment.
What's a little awkward about this is that this only yields generics, rather than the fully qualified type name. I understand that you don't really care about non-generic containing types, but the interface seems a little odd.
There was a problem hiding this comment.
What if I change the name to getGenericDeclaringType?
| } | ||
|
|
||
| /** Gets a textual representation of this type, taking nested types into account. */ | ||
| string toStringExt() { |
There was a problem hiding this comment.
toQualifiedString / toNestedString? Again, Ext is a little mysterious.
| } | ||
|
|
||
| /** Gets the number of children of this type, not taking nested types into account. */ | ||
| int getNumberOfChildrenSelf() { result = count(int i | exists(this.getChild(i)) and i >= 0) } |
There was a problem hiding this comment.
getNumberOfParametersSelf ?
This PR lets the unification library (and the "Implements" library) handle nested generic types. For example, types
A<int>.BandA<T>.Bare now correctly recognized as unifiable.I have started a
CSharp-Differencesjob here.