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

Ensure Mill names are only exported from one top-level package #3570

Open
lihaoyi opened this issue Sep 18, 2024 · 5 comments
Open

Ensure Mill names are only exported from one top-level package #3570

lihaoyi opened this issue Sep 18, 2024 · 5 comments
Milestone

Comments

@lihaoyi
Copy link
Member

lihaoyi commented Sep 18, 2024

Right now, importing both javalib._ and scalalib._ results in an error due to ambiguity when the same classes are exported from both:

package build
import mill._, javalib._, scalalib._

val x = Assembly
#06 [info] compiling 1 Scala source to /Users/lihaoyi/Github/mill/example/scalalib/basic/1-simple/out/mill-build/compile.dest/classes ...
#06 [error] /Users/lihaoyi/Github/mill/example/scalalib/basic/1-simple/build.mill:4:9: reference to Assembly is ambiguous;
#06 [error] it is imported twice in the same scope by
#06 [error] import scalalib._
#06 [error] and import javalib._
#06 [error] val x = Assembly
#06 [error]         ^
#06 [error] one error found

This isn't much of a problem now, as scalalib is a strict superset of javalib, so you can just use that all the time. But it may become a problem when #3567 breaks out javalib from scalalib so that they may each have their own unique features, and #3451 introduces kotlinlib._. When a user tries to have a multi-language codebase like import javalib._, scalalib._, kotlinlib._, they will inevitably hit the errors above

A solution to this would be to ensure that the different *lib packages do not export things that their upstream dependencies already export. So e.g.

  • If scalalib depends on javalib, you would need import javalib._, scalalib._ to work.
  • And similarly if kotlinlib depends on javalib, import javalib._, kotlinlib._.
  • And for a project with both java/scala/kotlin, import javalib._, kotlinlib._, scalalib._ would work without ambiguity

This will involve moving/removing a bunch of forwarders, and could happen together with #3567 in 0.13.0

@lihaoyi lihaoyi added this to the 0.13.0 milestone Sep 18, 2024
@jodersky
Copy link
Member

jodersky commented Dec 4, 2024

Regarding

When a user tries to have a multi-language codebase like import javalib., scalalib., kotlinlib._, they will inevitably hit the errors above

A solution to this would be to ensure that the different *lib packages do not export things that their upstream dependencies already export. So e.g.

What should we do with types that describe generic functionality and that are reimplemented in various packages? E.g. there could be a PublishModule shared across javalib, scalalib and kotlinlib, but implemented as a separate trait with the same name in pythonlib.

One approach would be to actively avoid name clashes, but I don't see how that scales without becoming very verbose (e.g. reinventing packages by prefixing the language PythonPublishModule).

Another would be to simply acknowledge that conflicts become possible, and encourage users to not use wildcard imports apart from the top-level mill._. Or at the very least if hey have more than one.

Personally I think that we can't always avoid conflicts and hence shouldn't use wildcard imports.
In general, I dislike wildcard imports since they hide the origin of definitions, yet are still required for code to work, and hence harm readability. Importing everything explicitly is also not the answer since it again harms readability by introducing a lot of verbosity and indirection ("what is Foo, scrolls up to wall of imports...., ah, it is in the same package as Bar, scrolls back down...").
Hence, although this is not commonly used in scala, I'm a fan of short import renames a la python (which has a very nice syntax in Scala3) and then explicit references. For example,

import mill.pythonlib as py
import mill.scalalib as sc

object foo extends py.PublishModule
object bar extends sc.PublishModule

// the imports are short enough, we could also just use the package directly
import mill.pythonlib
object foo extends pythonlib.PublishModule

This avoids ambiguity, yet keeps the number of imported definitions low. I don't know if encouraging something like this in more complex mill builds is reasonable, or if people would find it too strange.

@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 4, 2024

I think your suggestion could work. That's what the python folks do with import pandas as pd; import numpy as np and it seems to work decently. Something to explore once we break compat in 0.13.0

@lefou
Copy link
Member

lefou commented Dec 4, 2024

I think just avoiding re-exports is the correct thing to do. I was never a fan of the re-exports under javalib for exactly this reasons and I think I said that at the time of their introduction.

It's up to the user preferences and maybe the documentation author to choose between the different Scala import styles:

  • wildcard: import mill.scalalib.*
  • direct: import mill.scalalib.ScalaModule
  • prefix: import mill.scalalib
  • rename: import mill.scalalib as sc or import mill.scalalib.PublishModule as ScalaPublishModule

@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 5, 2024

The issue with avoiding re-exports is that you end up with import mill._, import javalib._, import scalalib._, import scalanativelib._, and the chain only gets longer as we break things down into more fine-grained components. It's doable, but it's pretty annoying. Of course, the current export conflict problem is annoying too, as is everything by py or sc or jv or whatever. I suspect there isn't going to be one obvious answer and we just need to pick our poison

@lefou
Copy link
Member

lefou commented Dec 5, 2024

We try to provide the most Language specific user experience, and having properly cut packages which of course requires the user to import it, is a consequence of Mill using Scala. Using Re-exporting/aliasing as a feature but having a compiler complaining about conflicting aliases looks like a feature to be avoided.

Since there is no easy migration path, except writing migration scripts/tooling, we should not apply that scheme to newly added classes. It make the jump to the next breaking Mill version just higher.

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

No branches or pull requests

3 participants