[Checkers] Checker Framework code review pre-comments

[Michael Ernst]

[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:
  <ul>
    ...
    <li>the java.lang.Class class
    <li>enum classes
    ...
  </ul>

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
Java comment:
  ...
  // 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 
https://groups.csail.mit.edu/pag/cert/logistics.html#order-of-procedures :

  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.

                    -Mike


TypeRelations.compareTypeArg

  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
  confusing.

  Why is the case of one annotation treated differently than the other
  cases?

  Please rename "TypeRelations" to "TypeHierarchy".

AnnotationRelations.isSubType(Collection, Collection)

  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").

Mahmood says:

  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.

InternedVisitor.isInvocationOfEquals

  Why does this check the return type?  The parameter types, not the return
  types, determines the overriding relationship, which is what you want to
  test.

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?

Mahmood says:

  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,  
  methods).

  We are interested in another form of equality, where two annotations  
  are equal if their types and their values are equal.  We had multiple  
  options:
  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.

AnnotatedTypeFactory.unify

  Why isn't this method in QualifierHierarchy (previously named
  AnnotationRelations)?  That seems to be where it belongs.

JavariVisitor.commonAssignmentCheck

  The comment suggests that BaseTypeVisitor should have a signature that
  accepts two AnnotatedTypeMirror parameters.  What do others think of
  this?  Let's discuss.

JavariAnnotatedTypeFactory

  There is no documentation in this, which is unacceptable.  As a result, I
  can't make heads or tails of it.

BasicChecker.UserTypeFactory

  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

  BaseTypeChecker should perhaps be renamed to TypeHierarchyChecker, which
  explains exactly what it does.

Flow

  I had a hard time understanding this class, so I wasn't able to read
  NonNullFlow.java