[Checkers] Checkers Code Review

[Jeff Perkins]

I started my  review with the documentation.  Attached are my comments.
Comments on the code to follow.

Note that I tried to read the documentation very carefully (as a
prospective checker writer).  I included everything that I found
even slightly confusing (even if I later figured it out). Please
disregard anything that seems unimportant.

-jeff

Manual:

  Overall:
    - Would it be possible to have a running example that could
      be referred to?  Much of the explanations are extremely
      technical (at least to me) and I would have have trouble getting
      started (without referring to an example).

    - It seems that a fair amount of detailed knowledge of the javac compiler 
      internals is assumed.  This may be necessary, but I think it may be
      off-putting to someone considering writing a checker.  If it were
      possible to lessen this, I think it would be much more encouraging to
      prospective checker writers.   A running example, might also serve
      to make this feel less daunting.

      My guess is that its really pretty easy to write a simple checker,
      but that is not the impression I get from reading the documentation.
      I'd probably figure that out if I looked at one of the examples,
      but I'd probably start with the documentation.

  9.0

    - in the 9.2 bullet, it might be helpful to include an example of
      a literal (I presume that "any string" and java.lang.String.class
      are examples).  I mention this both because the term may not be
      clear to everyone and I was confused because I tend to think of 
      literals as primitives (like 5 or 1.0) which don't really make
      sense for nonnull.

    - in the 9.3 bullet, are these rules in addition to those defined
      in the hierarchy?  Like the dereference of pointers that are not
      nonnull?  I think that is what is meant by '...your checker 
      automatically inherits such rules', but that wasn't clear to me.

  9.1

    - What is the @interface keyword? 

    - What is a meta-annotation?

    - What does it mean to say that the framework 'only handles types with
      one qualifier'?  I'm guessing this means that there can only be
      one qualifer on a single type (eg, it doesn't make sense to say
      @nonnull @null String), not that there can only be one qualifier
      in the type hierarchy, but on first reading that is not clear.  If
      i'm reading this correctly, it might make sense to downplay this as
      I doubt anyone would expect to put two qualifiers (from the same
      type hierarchy on a single instance of a type).

  9.1.1

    - Where are these annotations placed?  The first line says on the
      declaration of qualifier annotations.  The last paragraph says
      'on the checker class'.  What is the checker class?  I'm guessing
      this is pretty simple and an example would make this clear.  But
      I really don't know what files I'm supposed to create and how they
      interact with my code.

  9.1.2
    
    - What does the term 'invariant' mean in the next to last sentence?
      I presume that it means that the generic's type arguments must be 
      exactly the same (not a subtype of one another).  If this could be
      accurately described without requiring readers to remember the
      definitions of covariant, contravariant, and (I think, the less
      common) invariant, I think that would make this more clear.  I don't
      think I'm the only one who has to look up these terms most times
      I encounter them (it used to be every time, so I'm making progress).

      Would it be inaccurate to say:

        For instance, Java language specifications species that two
        generic types are subtypes only if their type arguments are
        identical.  The Javari type system overrides this behavior to
        allow a generic type to be a subclass of another generic if
        the first generics type arguments are subtypes of the second
        (e.g., List<@Mutable Date> is a subtype of List<@QReadOnly
        Date>).


  9.2.1

    - What do I need to know to understand this section?  For example, is
      there a list of applicable class literals for treeClasses?
      I presume that what I must do is understand the compiler types
      Tree.KIND, TypeKind, Tree, and TypeMirror.  But if there is a simpler
      way of figuring this out, it would certainly make this easier to
      do.  

      Perhaps understanding those classes in detail is essential for any
      checker, but if not, it would be great if we could provide (or
      reference) a list of reasonable choices and what they mean.

    - After the bullets the text reads 'the Nullable annotation is annotated...',
      Is this a meta-annotation?

    - How does the nonnull checker specifiy all literals except null
      (since I presume NULL_LITERAL) is a subtype of LiteralTree).  Or is
      this something that has to be specified in code?

  9.3
    - I think the third paragraph should begin with 'A checker's' rather than
      'The checker's'.  This certainly seems minor, but I find myself sometimes
      confused about what a checker writer is to do (which I consider 'a'
      checker) and something defined by the framework (possibly 'the' checker).

  9.4
    - Is the class 'TypeChecker' the class the checker writer creates that
      extends BaseTypeChecker?  I presume this is the case, but it is never
      explicitly stated.  Its also not in the code font, but is in the
      mixed case format, so I'm not sure what it is intended to be.  And
      is 'FooChecker' an example of TypeChecker?

    - What do I do in TypeChecker?  Do I need do anything more than annotate
      it with the TypeQualifiers annotation?  If this is really simple,
      an example would drive home how easy this is.




Michael Ernst wrote:
>> I updated the files in the same old location (~/mali/public_html/ 
>> checkers-code-review) to the latest checker code.
> 
> Warning:  checkers.pdf was not updated, but checkers.ps was.
> 
>                     -Mike