Mono.Cecil API Improvements...
Jonathan Pryor
Mono.Cecil, and (of course) found several "problems" with the API. :-)
First, some background: the FxDG [0] suggests that the API should be a
"pit of success" -- that the API naturally leads you to Do The Right
Thing (as opposed to being a mountain of pain, as The Right Thing is
difficult to do or difficult to find out that you need to do it).
Sadly, I've found Cecil to be more of a mountain than a pit. :-/
So, in no particular order...
Major Issues:
------------
It's a .NET 1.1 API. This makes things "ugly" when using C# 3 features
such as LINQ, as you can't just use e.g. assembly.Modules.Select(...),
but must instead insert a .Cast<T>() call, e.g.
assembly.Modules.Cast<ModuleDefinition>().Select(...).
This also causes `var' and `foreach' to not play nicely together, as the
inferred type is `object' (no help there).
Consequently, some wonderful #if logic would be nice so that under
NET_2_0 the base type is Collection<T> instead of CollectionBase.
Furthermore, some collection types should use KeyedCollection<TKey,
TValue> as the base type instead of Collection<T>, so that "useful"
indexing operations can be performed. Targets for this would be e.g.
TypeDefinitionCollection (with TKey=string),
AssemblyNameDefinitionCollection (TKey=AssemblyNameDefinition), etc.
This would allow easier lookup, e.g.
TypeDefinition d = assembly.MainModule.Types ["System.String"];
By far my biggest issue: the API is *too* OOP, where "too" means "it
doesn't work very well with intellisense," meaning "I need to know too
much to use the API."
Case in point, what do you do if you need to know if a type is an array?
For Reflection, you use the System.Type.IsArray.
For Cecil, you use a runtime check, e.g. `if (foo is ArrayType) ...`.
My problem with this is that it's not at all discoverable, you have to
_know_ that (1) ArrayType exists, and (2) what the semantics are for
ArrayType, and without documentation (2) is rather difficult to deduce
without reading *lots* of sample code (Gendarme) or the Cecil source
itself.
Then there's the documentation problem -- the TypeReference docs would
refer to the .IsArray property, and .IsArray would describe what it
does. Compare to the current approach where you'd need to document in
*both* TypeReference and ArrayType documentation the relation between
these two types (as TypeReference/TypeDefinition is the "entrypoint" for
most useful Type operations, and ArrayType will likely be overlooked, at
least *I* originally overlooked ArrayType...).
In short, FAIL.
Please, *please*, PLEASE follow Reflection in this regard: remove the
runtime type checking from user's code, and provide properties instead.
Code completion will thank you.
This applies to, at minimum, ArrayType, GenericParameter, PointerType,
ReferenceType, and TypeSpecification.
(So yes, I'm advocating the addition
of .IsArray, .IsGenericParameter, .IsPointer, .IsReference,
and .IsTypeSpecification properties on TypeReference.)
Reflection Compatibility Issues:
-------------------------------
I suspect that some code will be migrated from Reflection to Cecil, and
(assuming that's true) changing some Cecil semantics may make such a
migration easier. This would include:
AssemblyDefinition.GetTypes() should also return nested types.
The TypeReference.Namespace value for nested types should actually
contain a namespace value (not the empty string), instead of requiring
that we go to the (non-nested) declaring type (which may not be the
first .DeclaringType value, as types can be nested arbitrarily deep).
Minor Issues:
------------
ReferenceType vs. TypeReference. Completely different, yet very similar
names. Is there a way to improve this? (Perhaps by removing
ReferenceType and instead relying on a TypeReference.IsReference
property, as advocated above.)
I'm torn about GenericArgument and GenericParameter, as for mdoc-update
processing these two are handled almost identically, and iirc in
Reflection they're both represented by a System.Type
with .IsGenericParameter==true.
It would be convenient if the *Definition types would create a `new'
DeclaringType property which returns TypeDefinition, since (1) by
definition a e.g. FieldDefinition must have a TypeDefinition for it's
DeclaringType, and (2) this removes a cast that must dirty our code:
class TypeDefinition : TypeReference {
public new TypeDefinition DeclaringType {
get {return (TypeDefinition) base.DeclaringType;}
}
}
The use of properties results in some "puzzling"/odd-looking
expressions, e.g. methodDefinition.ReturnType.ReturnType to get the
actual return type as a TypeReference (as the first ReturnType returns a
MethodReturnType instance), and e.g. assemblyDefinition.Name.Name...
Which leads us to...
Naming. The Mono.Cecil type naming pattern leads to such "oddities"
mentioned above. I like the Collection, Definition, Reference, and
Specification suffixes, but the types that don't have consistent
suffixes result in odd expression chaining, as mentioned above. Perhaps
for an Info suffix should be used for
non-Definition/Reference/Specification types, and the Info name could be
used in the property name. Thus MethodReturnTypeInfo instead of
MethodReturnType, resulting in
methodDefinition.ReturnTypeInfo.ReturnType, which makes a tad bit more
sense. (Or perhaps .Returns, which would return a ReturnTypeInfo...)
Alternatively, methods/properties returning a TypeReference shouldn't
have a Type suffix, but instead a Ref suffix, e.g.
methodDefinition.ReturnType.ReturnTypeRef.
It would be useful if a TypeReference could have a link to the
AssemblyNameReference from which it came. Instead, I need to e.g.
AssemblyNameReference r = type.Module.AssemblyReferences
.Cast<AssemblyNameReference> ()
.Where (anr => anr.Name == type.Scope.Name)
.FirstOrDefault ();
(which also points out an over-reliance on string comparisons -- using
`anr == type.Scope' failed, while `anr.Name == type.Scope.Name' works;
I'm not sure how to work around this, but it's annoying.)
Thanks,
- Jon
[0]
http://www.amazon.com/Framework-Design-Guidelines-Conventions-Development/dp/0321545613