CLJS-3438: Inference for goog.object/containsKey returns any, not boolean (#262)

- fix cljs.analyzer/desugar-dotted-expr, generated malformed AST in the case of goog.module var
- compiler test for goog.object/containsKey
- fix parameter parsing in cljs.externs to properly handle var args and optional arguments
- fix fn-arity warning so that we use unaliased names if available (goog.module names are aliases)
- cljs.core: goog.object/containsKey hint no longer needed

Author: swannodette

src/main/cljs/cljs/core.cljs

@@ -12180,7 +12180,7 @@ reduces them without incurring seq initialization"
     (let [k (munge (str_ sym))]
       ;; FIXME: this shouldn't need ^boolean due to GCL library analysis,
       ;; but not currently working
-      (when ^boolean (gobject/containsKey obj k)
+      (when (gobject/containsKey obj k)
         (let [var-sym (symbol (str_ name) (str_ sym))
               var-meta {:ns this}]
           (Var. (ns-lookup obj k) var-sym var-meta)))))

src/main/clojure/cljs/analyzer.cljc

@@ -1214,9 +1214,12 @@
 
 (defmethod resolve* :goog-module
   [env sym full-ns current-ns]
-  {:name (symbol (str current-ns) (str (munge-goog-module-lib full-ns) "." (name sym)))
-   :ns current-ns
-   :op :var})
+  (let [sym-ast (gets @env/*compiler* ::namespaces full-ns :defs (symbol (name sym)))]
+    (merge sym-ast
+      {:name (symbol (str current-ns) (str (munge-goog-module-lib full-ns) "." (name sym)))
+       :ns   current-ns
+       :op   :var
+       :unaliased-name (symbol (str full-ns) (name sym))})))
 
 (defmethod resolve* :global
   [env sym full-ns current-ns]
@@ -3887,15 +3890,15 @@
         bind-args? (and HO-invoke?
                         (not (all-values? args)))]
     (when ^boolean fn-var?
-      (let [{^boolean variadic :variadic? :keys [max-fixed-arity method-params name ns macro]} (:info fexpr)]
-        ;; don't warn about invalid arity when when compiling a macros namespace
+      (let [{^boolean variadic :variadic? :keys [max-fixed-arity method-params name unaliased-name ns macro]} (:info fexpr)]
+        ;; don't warn about invalid arity when compiling a macros namespace
         ;; that requires itself, as that code is not meant to be executed in the
         ;; `$macros` ns - António Monteiro
         (when (and #?(:cljs (not (and (gstring/endsWith (str cur-ns) "$macros")
                                       (symbol-identical? cur-ns ns)
                                       (true? macro))))
                    (invalid-arity? argc method-params variadic max-fixed-arity))
-          (warning :fn-arity env {:name name :argc argc}))))
+          (warning :fn-arity env {:name (or unaliased-name name) :argc argc}))))
     (when (and kw? (not (or (== 1 argc) (== 2 argc))))
       (warning :fn-arity env {:name (first form) :argc argc}))
     (let [deprecated? (-> fexpr :info :deprecated)
@@ -3946,14 +3949,20 @@
                       {:op :host-field
                        :env (:env expr)
                        :form (list '. prefix field)
-                       :target (desugar-dotted-expr (-> expr
+                       ;; goog.module vars get converted to the form of
+                       ;; current.ns/goog$module.theDef, we need to dissoc
+                       ;; actual extern var info so we get something well-formed
+                       :target (desugar-dotted-expr (-> (dissoc expr :info)
                                                         (assoc :name prefix
                                                                :form prefix)
                                                         (dissoc :tag)
                                                         (assoc-in [:info :name] prefix)
                                                         (assoc-in [:env :context] :expr)))
                        :field field
                        :tag (:tag expr)
+                       ;; in the case of goog.module var if there is :info,
+                       ;; we need to adopt it now as this is where :ret-tag info lives
+                       :info (:info expr)
                        :children [:target]})
                     expr)
     ;:var

src/main/clojure/cljs/externs.clj

@@ -13,9 +13,9 @@
             [clojure.string :as string])
   (:import [com.google.javascript.jscomp
             CompilerOptions CompilerOptions$Environment SourceFile CompilerInput CommandLineRunner]
-           [com.google.javascript.jscomp.parsing Config$JsDocParsing]
+           [com.google.javascript.jscomp.parsing Config$JsDocParsing JsDocInfoParser$ExtendedTypeInfo]
            [com.google.javascript.rhino
-            Node Token JSTypeExpression JSDocInfo$Visibility]
+            Node Token JSTypeExpression JSDocInfo JSDocInfo$Visibility]
            [java.util.logging Level]
            [java.net URL]))
 
@@ -88,14 +88,13 @@
   (some-> (.getRoot texpr) parse-texpr simplify-texpr))
 
 (defn params->method-params [xs]
-  (letfn [(not-opt? [x]
-            (not (string/starts-with? (name x) "opt_")))]
-    (let [required (into [] (take-while not-opt? xs))
-          opts (drop-while not-opt? xs)]
-      (loop [ret [required] opts opts]
-        (if-let [opt (first opts)]
-          (recur (conj ret (conj (last ret) opt)) (drop 1 opts))
-          (seq ret))))))
+  (let [not-opt? (complement :optional?)
+        required (into [] (map :name (take-while not-opt? xs)))
+        opts     (map :name (drop-while not-opt? xs))]
+    (loop [ret [required] opts opts]
+      (if-let [opt (first opts)]
+        (recur (conj ret (conj (last ret) opt)) (drop 1 opts))
+        (seq ret)))))
 
 (defn generic? [t]
   (let [s (name t)]
@@ -108,6 +107,18 @@
       (= t 'Array) 'array
       :else t)))
 
+(defn get-params
+  "Return param information in JSDoc appearance order. GCL is relatively
+  civilized, so this isn't really a problem."
+  [^JSDocInfo info]
+  (map
+    (fn [n]
+      (let [t (.getParameterType info n)]
+        {:name      (symbol n)
+         :optional? (.isOptionalArg t)
+         :var-args? (.isVarArgs t)}))
+    (.getParameterNames info)))
+
 (defn get-var-info [^Node node]
   (when node
     (let [info (.getJSDocInfo node)]
@@ -124,15 +135,15 @@
               (if (or (.hasReturnType info)
                       (as-> (.getParameterCount info) c
                         (and c (pos? c))))
-                (let [arglist  (into [] (map symbol (.getParameterNames info)))
+                (let [arglist  (get-params info)
                       arglists (params->method-params arglist)]
                   {:tag             'Function
                    :js-fn-var       true
                    :ret-tag         (or (some-> (.getReturnType info)
                                           get-tag gtype->cljs-type)
                                         'clj-nil)
-                   :variadic?       (boolean (some '#{var_args} arglist))
-                   :max-fixed-arity (count (take-while #(not= 'var_args %) arglist))
+                   :variadic?       (boolean (some :var-args? arglist))
+                   :max-fixed-arity (count (take-while (complement :var-args?) arglist))
                    :method-params   arglists
                    :arglists        arglists}))))
           {:file *source-file*

src/test/clojure/cljs/compiler_tests.clj

@@ -391,6 +391,16 @@
                      (if (gstring/contains "foobar" "foo") true false)]))]
       (is (nil? (re-find #"truth_" code))))))
 
+(deftest test-goog-module-infer-cljs-3438
+  (testing "goog.object/containKey requires correct handling of vars from
+  goog.module namespace"
+    (let [code (env/with-compiler-env (env/default-compiler-env)
+                 (compile-form-seq
+                   '[(ns test.foo
+                       (:require [goog.object :as gobject]))
+                     (if (gobject/containsKey nil nil) true false)]))]
+      (is (nil? (re-find #"truth_" code))))))
+
 ;; CLJS-1225
 
 (comment

src/test/clojure/cljs/type_inference_tests.clj

@@ -403,14 +403,11 @@
                       (:require [goog.string :as gstring]))
                     (gstring/contains "foobar" "foo")]
                   {} true)))))
-  ;; FIXME: infers any instead of boolean, nothing wrong w/ the externs parsing
-  ;; but this definitely does not work at the moment
-  #_(is (= 'boolean
+  (is (= 'boolean
         (:tag
           (env/with-compiler-env (env/default-compiler-env)
             (ana/analyze-form-seq
               '[(ns test.foo
                   (:require [goog.object :as gobject]))
                 (gobject/containsKey (js-object) "foo")]
               {} true))))))
-