[Checkers] Checker Framework code review pre-comments

[Michael Ernst]

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.  Here is my list (in some cases, I've indicated what the issue is;
in others, I'll bring it up in person):

  AnnotationRelations.isSubType(Collection, Collection)

  InternedVisitor.isInvocationOfEquals

  GraphAnnotationRelations:  whole class must be rewritten from scratch.
    Particular offenders are .wildcard and .addSubtype (which is buggy).

  TypeRelations.copmareTypeArg

  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?

  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

  AnnotationUtils.isSame

  JavariVisitor.commonAssignmentCheck

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

  BaseChecker.UserTypeFactory


I'll talk with you at the code review on Monday.

                    -Mike