Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cljr-clean-ns when importing a deftype #194

Closed
aiba opened this issue Apr 11, 2017 · 5 comments · Fixed by #367
Closed

cljr-clean-ns when importing a deftype #194

aiba opened this issue Apr 11, 2017 · 5 comments · Fixed by #367
Labels

Comments

@aiba
Copy link
Contributor

aiba commented Apr 11, 2017

If namespace A has a deftype that is imported by namespace B, then namespace B needs to also require A, or the import could fail. For example:

(ns test.a)

(deftype MyType [])
(ns test.b
  (:require test.a)
  (:import test.a.MyType))

(def x (MyType.))

If you run cljr-clean-ns on test.b, it will remove the require of test.a. Without that require, the (deftype MyType []) is never evaluated, and loading test.b will produce a ClassNotFoundException.

Have other folks run into this / have a workaround?

One idea would be to add a rule to cljr-clean-ns: whenever path.to.SomeClass is imported, and there is a clojure namespace path.to, then also require path.to.

Environment & Version information

clj-refactor 2.3.0-SNAPSHOT (package: 20170407.553), refactor-nrepl 2.3.0-SNAPSHOT
CIDER 0.15.0snapshot (package: 20170403.402)
Clojure 1.9.0-alpha15
Leiningen 2.7.1 on Java 1.8.0_112
GNU Emacs 25.2.1 (x86_64-apple-darwin16.4.0, Carbon Version 157 AppKit 1504.81) of 2017-03-09
OS X 10.12.4
@expez
Copy link
Member

expez commented Apr 11, 2017

Is this a problem even after loading the code into the repl, e.g. with C-c C-k?

I just find it hard to believe that no-one has run into this until now!

I suppose it makes some sense... Even after writing clojure professionally for some time I've yet to use the deftype form.

Thanks for reporting!

@expez expez added the bug label Apr 11, 2017
@aiba
Copy link
Contributor Author

aiba commented Apr 11, 2017

Is this a problem even after loading the code into the repl, e.g. with C-c C-k?

If you manually load test.a with C-c C-k, then yes, the deftype is evaluated and test.a.MyType is generated so the import would work without a require. However, if you then did a clojure.tools.namespace.repl/refresh, it would reload the code in dependency order, and since nothing requires test.a, it would fail.

I just find it hard to believe that no-one has run into this until now!

I believe it. A common pattern with deftype (and other dynamically-generated java classes) is to provide a clojure function that wraps the class constructor. So consumers would require/refer the clojure function that constructs the type, rather than importing class.

Another reason others might not have run into this is that often namespaces that have a deftype have other stuff in them which could cause that namespace to happen to be required before any of its generated classes are imported.

Unfortunately in my case, I need to import the actual class and just the class to use as a type hint to avoid a costly reflection call.

If I were hand-writing the namespace requires, I'd just keep the extra (:require test.a) before the (import test.a.MyClass). But cljr-clean-ns will remove the require.

Another possible solution that is a little more general: we could have a way of marking requires as "do not remove" by cljr-clean-ns. Maybe with metadata.

(ns foo.c
  (:require [foo.a]
            ^:cljr-keep [foo.b]))

Just a thought.

@aiba
Copy link
Contributor Author

aiba commented Aug 6, 2019

Just commenting here to note that this issue is still unrsolved, but I had an idea for how to resolve it. It's a bit of a heuristic, but I can't think of cases where it would go wrong.

What if, when cleaning a namespace, we keep all requires that are substrings of any imported classes? For example, if we require myproj.a.b and also import myproj.a.b.Foo, then we will keep the require even if it's not used, because we assume it's necessary for the import.

@expez
Copy link
Member

expez commented Aug 7, 2019

Yeah, this is definitely solvable. We could also look for explicit constructor use. I just haven't gotten to this because I never write code like this. I always make a ->MyType constructor that I use outside the ns.

@vemv
Copy link
Member

vemv commented Feb 10, 2022

Fixed in refactor-nrepl 3.3.2 / clj-refactor.el 3.3.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants