mranderson icon indicating copy to clipboard operation
mranderson copied to clipboard

Over-zealous replacing imports inlining potemkin 0.4.5

Open jarohen opened this issue 3 years ago • 3 comments

Hey again @benedekfazekas :smile:

Looks like mranderson is replacing imports that it shouldn't when it needs to rename only some classes within a package.

repro:

  • ^:inline-dep [potemkin "0.4.5"] - this has dependencies on riddley "0.1.12" and clj-tuple "0.2.2"; it's the presence of both of these that causes the issue. (I'm trying to inline clj-http "3.12.2", but it seems potemkin is a smaller repro.)
  • lein inline-deps
  • check the outputted riddley.compiler - mranderson has prefixed clojure.lang.Var etc, but these don't appear prefixed in class-deps.jar.
  • check class-deps.jar - there are other clojure.lang classes present from clj-tuple.

I reckon this is something to do with prefix-dependency-imports! using a set of packages to replace - is there anything preventing us from using exact classes instead?

Thanks again!

James

jarohen avatar Jun 22 '21 22:06 jarohen

Cheers! :pray:

jarohen avatar Jul 14 '21 10:07 jarohen

Ah, sorry @benedekfazekas - I don't think we can just filter out clojure.lang like this:

  • We don't want to rename clojure.lang.Var etc - these are in the standard Clojure distribution (at least, I don't know of a reason to want this - do people want this to bring in different versions of Clojure, maybe?)
  • We do want to rename clojure.lang.PersistentUnrolledVector etc - these are classes in the clj-tuple library.

Maybe we can check for presence in the class-deps.jar? clojure.lang.PersistentUnrolledVector is present, but clojure.lang.Var isn't, in this case.

jarohen avatar Jul 14 '21 16:07 jarohen

hm... apparently I was too hasty with this. maybe needs a bit more thinking time. As a general rule I don't want to inline clojure core stuff but clj-tuple is special in this regard obviously. reopening this.

it is also tricky because all cloure.lang are prefixed so mranderson would need to split this or rewrite this prefixing to achieve what you are suggesting I think.

benedekfazekas avatar Jul 15 '21 15:07 benedekfazekas