[Checkers] Checker Framework code review pre-comments
mernst at csail.mit.edu
Mon Jun 2 04:39:37 EDT 2008
[This is a re-send of my previous email, but with Mahmood's responses
integrated (thanks, Mahmood!) and with a couple sentences of explanation
about each of the items in my list.]
Matt and Mahmood-
I have read the (voluminous) code that you sent me for the review. (I
didn't read NonNullFlow because the printouts didn't include Flow, which is
necessary in order to understand NonNullFlow.)
Overall, I was pretty happy with the amount of documentation in the code.
I could understand most parts of it. (There were some glaring
counterexamples, like the Javari checker that was missing large amounts of
essential documentation.) As a minor point, please don't add useless
comments that just repeat the name of the parameter and its type, or in
which the @returns clause is essentially identical to the method summary.
Comments should enlighten, not merely repeat.
I have a suggestion regarding how to make the helpful comments in the
checkers/visitors/factories even more helpful. Currently, they are
organized as HTML lists:
<li>the java.lang.Class class
Instead of using automatic numbering, number the clauses by hand:
4. the java.lang.Class class
5. enum classes
If there were multiple HTML lists in one Javadoc, number sequentially
rather than starting subsequent lists from 1 again.
Then, wherever in the class that a particular rule is implemented, add a
// Case 4. the java.lang.Class class
// Case 5. enum classes
This will make it much easier to understand the purpose of each part of the
code, and to see whether anything has been omitted from the documentation
or from the code.
I found several of the classes unnecessarily difficult to read because the
methods in the class were not organized in a logical fashion that made them
easy to understand. So I added the following text to
Order of procedures
When you add a new procedure to a file, don't just type it wherever your
cursor happens to be. Instead, place related methods together. It is often
helpful to put a block comment (e.g., starting with a row of 75 asterisks)
at the beginning of each group of related methods. Such block comments
divide the file into sections that are readily apparent to readers.
In general, put public methods before private ones in your files. Organize
your file with helper methods (whether public or private) after the main
entry points. This permits readers to read your code top-down, which is
more comprehensible: the purpose of each piece of code, and how it fits
into the whole, is obvious. A reader can forward-reference to just the
specification, not the whole implementation, of a helper method. (This
doesn't mean you necessarily have to write your in a code top-down order,
but do organize it that way for readers.)
I have a marked-up copy of the code that I will give you, but I won't be
able to scan it to PDF until Monday morning. Then, you can divvy up the
comments and work from a color printout. I strongly suggest against
working from an electronic copy, because you will miss some of the
comments. Perhaps not every suggested change is a good idea, but do please
examine them all. Then, for ones that aren't a good idea, you can explain
to me why. That will improve my understanding (either of this code or in
general), and we can decide whether some other change (possibly to the
documentation instead or in addition) is superior, or I was just confused.
This step of us discussing the suggested changes is quite valuable for all
sides, since it's part of the learning process that makes everyone
(reviewers and reviewees alike) better coders. I consider this extremely
important even if some of you won't be around for a long time, because
improving your skills is part of both your and my jobs at MIT.
As is usual for our code reviews, each reviewer (in this case, Jeff, Telmo,
and me) will have a set of specific questions that seem worth discussing in
person. Below is my list (in some cases, I've indicated what the issue is;
in others, I'll bring it up in person). I'll talk with you at the code
review on Monday.
This method name is unfortunate, because it (vaguely) says one way to use
the method (when comparing), but gives no indication of what it does.
The method name and Javadoc use "compare", "similar", and "same", none of
which conveys any information to readers (and at least one of which,
"same", seems to be actually incorrect). Rename it to a name that
contains "subtype" or whatever it actually does. Similar comments apply
to other methods in the class.
Avoid reassigning formal parameters in method bodies; that is usually
Why is the case of one annotation treated differently than the other
Please rename "TypeRelations" to "TypeHierarchy".
The arguments are in the wrong order for an "isSubtype" method.
Also, the semantics are wrong: subtyping requires that every annotation
in rhs is a subtype of some annotation in lhs.
Rename every use of "AnnotationRelations" to "QualifierHierarchy"
(including "GraphAnnotationRelations" to "GraphQualifierHierarchy").
The specification is a bit odd, since it returns true if any
annotation in the first collection is a subtype to one annotation in
the second collection. I had two cases while thinking about it:
1. Javari for a while had RoMaybe along with other annotations.
Having isSubtype(Collection, Collection) enabled us to let
JavariChecker uses AnnotationRelations easily. We didn't have this
method in the first design.
2. As an implementation detail, IGJ allows @AssignsFields and @I to be
in the same type. A self type (type of 'this') may be @AssignsFields
@I FOO, where AssignsFields would allow for invocation of other
AssignsFields methods and @I would allow 'this' reference to escape as
'@I'. This could easily be changed.
Why does this check the return type? The parameter types, not the return
types, determines the overriding relationship, which is what you want to
GraphAnnotationRelations: whole class must be rewritten from scratch.
Particular offenders are .wildcard and .addSubtype (which is buggy).
I don't think that root and wildcard are well-motivated: it's little or
no trouble for a client to specify all the relationships, and that
simplifies both the interface and the implementation of the class.
"Placeholder" is an unintuitive name that is not explained; eliminate it
from the code.
Additionally, the class should be split into two parts: construction and
querying. Either the first query, or else an explicit method call,
should transition between those parts. At that time, the relationships
should be computed from the graph, and thereafter further construction
should be illegal (throw an exception). This will make the class simpler
to understand and use, and the implementation will be less error-prone.
The current "inConflict" should be named "redundant"; it doesn't actually
indicate a conflict. An example of a conflict is a String representing a
method signature that is annotated both as JVML and as Java format. The
current implementation doesn't seem to support a true notion of
"inConflict". I think it should, via user-specified method calls similar
to "addSubtype". That should probably be promoted to QualifierHierarchy
rather than staying in GraphQualifierHierarchy.
For annotations (AnnotationMirror) what is the difference between
canonical names and qualified names? Why are names used for equality
checking, rather than overriding the AnnotationMirror.equals method?
What is the contract of equality for AnnotationMirror?
How is AnnotationUtils.isSame related to all this?
This is a bit complicated issue. There are two classes of interest
here: AnnotationMirror (and its implementation AnnotationMirrorImpl),
AnnotationValue (and its implementation AnnotationValueImpl). Both
classes are symbols of annotations, but each instance represent a
unique annotation declaration and its value; so two instances are not
equal if they have the same annotation type and values but are for
different declaration (e.g. annotations for different variables,
We are interested in another form of equality, where two annotations
are equal if their types and their values are equal. We had multiple
1. Create a new annotation representation to wrap all AnnotationMirror
coming from the compiler.
2. Create new set of equality methods. A simple one was using the
qualified name and the AnnotationMirrorImpl, AnnotationValueImpl,
implement a nice toString().
BaseTypeChecker vs. SourceChecker: Should all of the framework
functionality (such as Annotated*) rely on BaseTypeChecker instead of
SourceChecker? From a few casts, it seems like that is the case. Our
framework wouldn't be so useful for checkers not built on SourceChecker
in that case. Or, alternately, some functionality could be moved from
BaseTypeChecker into SourceChecker. I would also like to see some
documentation in SourceChecker regarding when one would want to use it
without using BaseTypeChecker; I think that would clarify things for me.
The purpose of SourceChecker is not particularly clear now.
Why isn't this method in QualifierHierarchy (previously named
AnnotationRelations)? That seems to be where it belongs.
The comment suggests that BaseTypeVisitor should have a signature that
accepts two AnnotatedTypeMirror parameters. What do others think of
this? Let's discuss.
There is no documentation in this, which is unacceptable. As a result, I
can't make heads or tails of it.
Should this functionality be in AnnotatedTypeFactory? More generally,
why isn't it built into the framework? It seems that many checkers could
take advantage of it.
BaseTypeChecker should perhaps be renamed to TypeHierarchyChecker, which
explains exactly what it does.
I had a hard time understanding this class, so I wasn't able to read
More information about the checkers