letfn collisions across namespaces

Question

If you letfn a fn with the same name in two namespaces, the wrong fn is used.

`
(ns hello-world.one)

(letfn [(answer [] "1")]
(defn get-answer []

(answer)))

`

`
(ns hello-world.two)

(letfn [(answer [] "2")]
(defn get-answer []

(answer)))

`

`
(ns hello-world.core
(:require [hello-world.one]

        [hello-world.two]))

(println "one =>" (hello-world.one/get-answer)) ; one => 1
(println "two =>" (hello-world.two/get-answer)) ; two => 1 WHAT?!?
`

This issue seems to exist both on top-level letfns and non-top-level (let (link: ) (letfn (link: ...))).

Answer 1

Comment made by: jeremyrsellars

This patch wraps letfn :expr and :statement forms in a function declaration (formerly, only :expr forms were wrapped).

I only did minimal testing. It fixes the code from the description.

Note: This is my first crack at the compiler and it is entirely possible I have not understood the entire problem.

Answer 2

Comment made by: mfikes

Confirmed that this fixes things downstream in self-hosted ClojureScript (Planck):

Without the patch:

cljs.user=> (require 'hello-world.core) one => 2 two => 2 nil

With the patch:

cljs.user=> (require 'hello-world.core) one => 1 two => 2 nil

Answer 3

Comment made by: dpsutton

This doesn't require different namespaces. The bug is that let-fn is putting its binding as a global variable.

And easy reproduction is
1. lein new mies letfn-bug,
2. update cljs version to (link: org.clojure/clojurescript "1.9.946")
3. and then

`
(ns letfn-bug.core
(:require [clojure.browser.repl :as repl]))

(enable-console-print!)

(letfn [(non-unique-name [] 4)]
(defn f1 [] (non-unique-name)))

(letfn [(non-unique-name [] 5)]
(defn f2 [] (non-unique-name)))

(println "should be 4-> " (f1))
(println "should be 5-> " (f2))
`

then scripts/repl.

results in:

`
cljs.user=> (load-file "letfn_bug/core.cljs")
should be 4-> 5
should be 5-> 5
nil
cljs.user=>

`

With the generated js:

// Compiled by ClojureScript 1.9.946 {} goog.provide('letfn_bug.core'); goog.require('cljs.core'); goog.require('clojure.browser.repl'); cljs.core.enable_console_print_BANG_.call(null); var non_unique_name = (function letfn_bug$core$non_unique_name(){ return (4); }); letfn_bug.core.f1 = (function letfn_bug$core$f1(){ return non_unique_name.call(null); }); var non_unique_name = (function letfn_bug$core$non_unique_name(){ return (5); }); letfn_bug.core.f2 = (function letfn_bug$core$f2(){ return non_unique_name.call(null); }); cljs.core.println.call(null,"should be 4-> ",letfn_bug.core.f1.call(null)); cljs.core.println.call(null,"should be 5-> ",letfn_bug.core.f2.call(null));

Answer 4

Comment made by: dnolen

Quick review of the patch, instead of always wrapping in a statement context it might be better if we only did it at the top-level where this is actually a problem.

Answer 5

Comment made by: gnl

I'd just like to chime in and suggest that the priority on this be raised. A compiler breaking scope like this is certainly not a minor issue.

Answer 6

Comment made by: jeremyrsellars

This patch is like the first, but only function-wraps statement letfn forms that are outside a fn-scope. (CLJS-1965-Wrap-top-level-letfn-to-avoid-collisions.patch)

Answer 7

Comment made by: timothypratley

I confirmed locally that the new patch works well... but I had to manually apply the changes because git apply fails. If Jeremy creates a fresh patch, does this look good to go? Let me know if I can help in any way (I'm happy to update his patch so he retains credit if that helps?)

Answer 8

Comment made by: dnolen

Wrapping in closures at the top-level might introduce other undesirable interactions with Closure advanced - before proceeding with the proposed patch I'd like to see whether DCE etc still works for this pattern.

An alternative is to track this pattern and assign to the namespace + shadowing logic (i.e. same name used multiple times in one ns).

Answer 9

_Comment made by: jeremyrsellars_

Here is an update to the Wrap-top-level-letfn-to-avoid-collisions patch current as of r1.10.520.

I couldn't think of a way to test dead code elimination with deftest, but we would expect the advanced compilation output of this to contain "kwc", but not contain "dce".

{code:title=letfn_dce_test.cljs|borderStyle=solid}
(ns cljs.letfn-dce-test
  (:require [cljs.test :refer-macros [deftest is]]))

(letfn [(fn-to-kwc [] :kwc)
        (fn-to-dce [] :dce)]  ; this fn & kw should be eliminated by DCE
  (defn f0 [] (count (str f (name (fn-to-kwc)))))
  (deftest letfn-live-code-test
    (is (pos? (f0)))))


I observed that it behaved as if dead code elimination still functioned.


bin/cljsc src/test/cljs/cljs/letfn_dce_test.cljs  "{:optimizations :advanced}"|less


David Nolan's alternative is intriguing.  Are there more reasons to pursue attaching the statement letfns to the namespace instead?

Answer 10

Reference: https://clojure.atlassian.net/browse/CLJS-1965 (reported by jeremyrsellars)