jsdoc parsing fails to recognize union types, breaking resolution

Question

The Closure [Spec For Union Types|https://developers.google.com/closure/compiler/docs/js-for-compiler#unions] states that parentheses are necessary for union type expressions. Trying this ...



(defn foo
  "@param {(IBar|IMap)} x"
  [x]
  ...)

 
Raises a Closure Error :


...ERROR - Bad type annotation. expected closing }
* @param {user.(IBar|user.IMap)}


This is because comp/resolve-types treats the parentheses as a part of the type tokens and incorrect var resolution occurs as a result. In addition, the compiler emits multiple resolved types separated by "|" characters but does not enclose them in parentheses to create a valid union type.

Answer 1

Comment made by: pkillean

This patch includes:

• comp/resolve-types now removes parentheses when present and emits them when >1 type is detected. This makes parenthesis use optional and existing code remains unbroken (with the added benefit that it may work now)

• changes to comp/resolve-type
  1. checks for js globals like document or window which are recognized by closure
  1. allows dot.delimited.forms to pass through so we can use types defined in externs and avoid unnecessary resolution
  1. uses ana/resolve-existing-var with a "unresolved jsdoc type" warning
  1. checks if a resolved var is a protocol and warns otherwise. This is more informative than Closure's standard unrecognized type error

• a test for comp/resolve-types

Answer 2

Comment made by: dnolen

Thanks will try to look more closely at this tomorrow.

Answer 3

Comment made by: dnolen

The patch is getting there, please remove the js-doc-type meta stuff. Just extend the signature of {{resolve-existing-var}} to take an additional parameter - the {{confirm-var-exists}} handler.

Answer 4

Comment made by: pkillean

CLJS-1627-1.patch:
{{resolve-existing-var}} now has an additional arity that accepts a missing-var handler passed to {{confirm-existing-var}}

Answer 5

Comment made by: pkillean

This has revealed a problem where {{deftype}} + {{defrecord}} using Object protocols emit resolved names when really they shouldn't. For example : "@implements {cljs.core.async.impl.timers.Object}" --> Bad Type Annotation

Since {{Object}} is a special case simply excluding it from the comments should fix it. Another patch incoming

Answer 6

Comment made by: pkillean

CLJS-1627-2.patch:
The emit* methods for deftype and defrecord now filter out Object protocols.

This produced an interesting result! With no more bad type annotations, static analysis can now proceed... and it has alot to say. Theres all kinds of info now about arity discrepencies (particularly cljs.core.IndexedSeq), type mismatches, and more. It even includes a type coverage percentage. Lots to parse here but very cool.

Answer 7

Comment made by: pkillean

CLJS-1627-3.patch:
fix require extern
add type application support for Array & Object
GC likes uppercase for Object & Array, lowercase for string, number.
support for explicit nullable types, variable typed arg
* function type context modifiers {{this}} + {{new}}

-Missing is the GC 'record type'- . It also may be useful to fill out the node externs for common types

Answer 8

Comment made by: pkillean

CLJS-1627-4.patch:
fix a few problems in last patch
add record type support. Everything (link: https://developers.google.com/closure/compiler/docs/js-for-compiler#types text: here) should be covered

Answer 9

Comment made by: pkillean

update patch

Answer 10

Comment made by: mfikes

CLJS-1627-5.patch no longer applies

Answer 11

Comment made by: pkillean

patch 6:
routes js comments through comp/emit-comment which has been altered to handle inline comments.
Supported tags: param, return, type, implements, typedef, enum, extends, throws, lends, const, this
add macro core/goog-typedef. This lets you name a custom + type refer to it in annotations.

More work is needed to support multi-arity fns, but I think this pretty much unlocks basic static type checking :-)

Answer 12

Comment made by: mfikes

Hey Patrick, CLJS-1627-6.patch does not apply to master.

Answer 13

Comment made by: pkillean

shoulda had coffee first

Answer 14

Comment made by: mfikes

CLJS-1627-7.patch doe not apply to current master

Answer 15

Reference: https://clojure.atlassian.net/browse/CLJS-1627 (reported by pkillean)