Sitelet https://github.com/cloverage/cloverage/pull/366
Skip to content

Fix instrumentation causing spurious reflection warnings - #366

Open
imrekoszo wants to merge 1 commit into
cloverage:masterfrom
imrekoszo:reflection-fixes
Open

imrekoszo wants to merge 1 commit into
cloverage:masterfrom
imrekoszo:reflection-fixes

Conversation

@imrekoszo

Copy link
Copy Markdown

Instrumented code emitted reflection warnings that the same code compiled without instrumentation doesn't:

  • propagate-fn-call-tag copied a var's :tag onto the instrumented call, but core vars carry a Class there and the compiler only honours symbol or string tags. Convert it to a symbol. Also pick up return type hints on arglists, e.g. (defn f ^String [x] ...), for the matching arity.

  • Since Clojure 1.12 macroexpand-1 no longer rewrites (Class/method ...) into a . form, so it got instrumented as a fn call with a wrapped head, a reflective qualified method value. Rewrite it to the . form first.

  • The :dotjava method rebuilt . forms without the original metadata, dropping type hints and line numbers. Keep it.

  • instrument evaluates forms one by one instead of going through load, so a namespace's (set! warn-on-reflection ...) leaked into every namespace instrumented after it. Rebind per namespace as load does.

  • You've updated the changelog (if adding/changing user-visible functionality)

Fixes #102 (hopefully - we have a large internal project that used to produce tons of reflection warnings with Cloverage, especially around core.async, and is clean with this change)

Disclaimer: I relied heavily on Copilot to implement this.

Repro case:

sh -c '
cat > deps.edn <<\EOF
{:paths ["."]
 :deps {org.clojure/clojure {:mvn/version "1.12.6"}}
 :aliases
 {:run
  {:main-opts ["-m" "cloverage.coverage" "--ns-regex" "a|b" "--test-ns-regex" "a-test" "--no-html" "--no-summary"]}
  :before
  {:extra-deps
   {cloverage/cloverage
    {:git/url "https://github.com/cloverage/cloverage"
     :git/sha "61e3cac426e9907a9dd01c37597f85c71a57ff90"
     :deps/root "cloverage"}}}
  :after
  {:extra-deps
   {cloverage/cloverage
    {:git/url "https://github.com/imrekoszo/cloverage"
     :git/sha "1160ed138a8eca4cc4673bfafa3ddf6af864305f"
     :deps/root "cloverage"}}}}}
EOF

cat > a.clj <<\EOF
(ns a
  (:import (java.util.concurrent.atomic AtomicReference)))

(set! *warn-on-reflection* true)

(defn class-tag
  "the pr-str var has a Class :tag"
  [x]
  (.length (pr-str x)))

(defn- hinted ^String [x] (str x))

(defn arglist-tag
  "return hint on the arglist"
  [x]
  (.length (hinted x)))

(defn static-call
  "Clojure 1.12 Class/method"
  [^String s]
  (.booleanValue (Boolean/valueOf s)))

(defn dot-form-meta
  "hint on a . form"
  [^AtomicReference r]
  (.length ^String (.get r)))
EOF

cat > b.clj <<\EOF
(ns b
  "No (set! *warn-on-reflection* true) here, so its reflection is not reported by a plain load."
  (:require [a]))

(defn leak [x]
  (.length x))
EOF

cat > a_test.clj <<\EOF
(ns a-test
  (:require [clojure.test :refer [deftest is]]
            [a]
            [b]))

(deftest smoke
  (is (= 1 (a/class-tag 1)))
  (is (= 1 (a/arglist-tag 1)))
  (is (true? (a/static-call "true")))
  (is (= 1 (a/dot-form-meta (java.util.concurrent.atomic.AtomicReference. "x"))))
  (is (= 1 (b/leak "x"))))
EOF

cat > run.sh <<\EOF
#!/usr/bin/env sh
# Counts reflection warnings for a plain load vs. cloverage before/after the fix.
cd "$(dirname "$0")" || exit

warnings() { grep -E "Reflection warning|Error|Exception|assertions|failures" | sed "s/^Reflection warning, //" ; }

echo "== plain load of b.clj (expected: none) =="
clojure -M -e "(require (quote b))" 2>&1 | warnings

cloverage() {
  echo "== cloverage $1 ($2) =="
  clojure "-M:$1:run" 2>&1 | warnings
}

cloverage before "expected: 5 in a.clj (due to instrumentation), 1 in b.clj (due to *warn-on-reflection* leaking from a.clj to b.clj)"
cloverage after "expected: none"
EOF

chmod +x run.sh && ./run.sh
'

Instrumented code emitted reflection warnings that the same code compiled
without instrumentation doesn't:

- propagate-fn-call-tag copied a var's :tag onto the instrumented call,
  but core vars carry a Class there and the compiler only honours symbol
  or string tags. Convert it to a symbol. Also pick up return type hints
  on arglists, e.g. (defn f ^String [x] ...), for the matching arity.
- Since Clojure 1.12 macroexpand-1 no longer rewrites (Class/method ...)
  into a . form, so it got instrumented as a fn call with a wrapped head,
  a reflective qualified method value. Rewrite it to the . form first.
- The :dotjava method rebuilt . forms without the original metadata,
  dropping type hints and line numbers. Keep it.
- instrument evaluates forms one by one instead of going through load,
  so a namespace's (set! *warn-on-reflection* ...) leaked into every
  namespace instrumented after it. Rebind per namespace as load does.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cloverage instrumentation breaks type hints, making tests run differently

1 participant