The Wayback Machine - https://web.archive.org/web/20200908105516/https://github.com/scala-js/scala-js/pull/4087/
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

WiP Remove the dependency from linker-interface on scalajs-ir. #4087

Draft
wants to merge 1 commit into
base: master
from

Conversation

@sjrd
Copy link
Member

sjrd commented Jun 17, 2020

And since sbt-scalajs only depends on linker-interface, this transitively removes the dependency from sbt-scalajs on scalajs-ir as well.

And since sbt-scalajs only depends on linker-interface, this
transively removes the dependency from sbt-scalajs on scalajs-ir as
well.
@sjrd
Copy link
Member Author

sjrd commented Jun 17, 2020

@gzm0 This needs a serious cleanup and decomposition in meaningful commits. At this point I'd like your opinion on whether this is something worth doing at all.

This allows the sbt plugin, along with any other build tool depending on the linker-interface, not to have any dependency on scalajs-ir. Currently, the fact that we receive scalajs-ir on the classpath because of linker-interface, but that we then load scalajs-linker reflectively, means that the version of scalajs-linker-interface must be exactly equal to that of scalajs-linker. While that is fine for sbt-scalajs, which is co-developed with scalajs-linker, it can be problematic for other build tools. Indeed, they are typically not built for one specific version of scalajs-linker, but instead load the right one depending on a setting. Nevertheless, they typically have to statically link against one version of scalajs-linker-interface. This means that it is very difficult for them not to encounter clashes.

On principles as well, when depending on scalajs-linker-interface we were automatically get scalajs-ir, whose versioning policy is equal to interface.unstable, yet its package name does not make that clear.


There are some difficulties, the most notable one being the fact that IRFileImpl must use "untyped" definitions for def entryPointsInfo and def tree. Previously they were ir.EntryPointsInfo and ir.Trees.ClassDef, but that is not available anymore. I made them completely abstract types, and introduced linker.standard.ConcreteIRFileImpl to safely cast between the abstract types and the concrete IR types.

@gzm0
Copy link
Contributor

gzm0 commented Jun 17, 2020

but that we then load scalajs-linker reflectively, means that the version of scalajs-linker-interface must be exactly equal to that of scalajs-linker

Isn't this the case anyways because of interface.unstable?

@sjrd
Copy link
Member Author

sjrd commented Jun 17, 2020

Yes and no. interface.unstable is, in practice, much more stable than ir.*. ir.* changes every time we introduce a new IR node or change the IR in any way. It also contains the IR version numbers themselves.

It is practical to program against the binary interface of scalajs-linker-interface and link with many different versions of scalajs-linker. It's not practical to link scalajs-linker with a version of scalajs-ir that is not exactly equal.

At least that's my impression.

@gzm0
Copy link
Contributor

gzm0 commented Jun 17, 2020

That's fair. So then to me, the major challenge is that the current linker interface assumes that IR file implementations are valid independently from the linker being used. (This is not an entirely hypothetical scenario, one could imagine using the cache of one version with another linker).

If we are really strict, this is something we have committed to, in the public interface. (I did, at some point, toy with path-dependent types for these things, but rejected it, because the typing would be a nightmare in sbt).

An idea of how to address this:

  1. Change the unstable API of IRFiles to rely on byte blobs, rather than IR.
  2. At this point, the cache is useless, as it cannot deserialize anymore.
  3. Create a cached linker type, that allows to create linkers from a cache (this would likely also make a IRFileCache.Cache like type obsolete).

We'd have to leave the existing type definitions around, but we can make them less efficient (basically only have them work on byte blobs).

The downside is the funky API, where you cannot create a single linker, you have to create a CachedLinkerFactory or whatever and use that to create linkers. (OTOH, IRFileCache would disappear from the non-deprecated interface).

@sjrd
Copy link
Member Author

sjrd commented Jun 18, 2020

So then to me, the major challenge is that the current linker interface assumes that IR file implementations are valid independently from the linker being used. (This is not an entirely hypothetical scenario, one could imagine using the cache of one version with another linker).

If we are really strict, this is something we have committed to, in the public interface.

Have we? I don't think we have, because at the moment it's simply impossible to have two different scalajs-linker's at all (at least not without loading two different scalajs-linker-interface's as well, in which case the whole thing here is a non-issue).

To make sure we are talking about the same thing, when you say "independently from the linker being used", are you talking about a) the instance of Linker or b) the classloaded scalajs-linker artifact? From:

(I did, at some point, toy with path-dependent types for these things, but rejected it, because the typing would be a nightmare in sbt).

it seems you meant a). But in the a) case, nothing changes with this PR. We're still able to pass (cached) IR files to different instances of Linker. We just have to make sure that they come from the same classloaded scalajs-linker, but I think that's a fair requirement that is quite common in JVM classloaded artifacts.

@gzm0
Copy link
Contributor

gzm0 commented Jun 18, 2020

Have we? I don't think we have, because at the moment it's simply impossible to have two different scalajs-linker's at all

In the context of loading different artifacts, yes. But different linker implementations is absolutely possible:

val myFiles = PathIRFile(...)
val irCache = StandardImpl.irFileCache()
val stdLinker: Linker = StandardImpl.linker(config)
val myAwesomeLinker: Linker = new MyAwesomeLinker()

stdLinker.link(irCache.cached(myFiles))
myAwesomeLinker.link(irCache.cached(myFiles))

If myAwesomeLinker has a different IR, with this PR, this will throw a CCE somewhere. Note that for this example to fail, we do not even need any classloading at all.

@sjrd
Copy link
Member Author

sjrd commented Jun 18, 2020

Right. That's in theory possible, indeed. I don't think it's a valid use case, though. In my mind the abstract types in IRFileImpl are abstract proxies for ir.EntryPointsInfo and ir.Trees.ClassDef only. That's why the casting functions are in standard.*. Casting them to something else than the IR classes in scalajs-ir is incorrect, in my view of this API.

@gzm0
Copy link
Contributor

gzm0 commented Jun 18, 2020

Hum, but your argument requires implementation details from the point of view of linker.interface. That shouldn't be the case. Whatever linker.interface allows you to do should be possible, regardless of implementation details.

If casting the abstract types to something else is incorrect, the interface giving access to it should not allow it.

Maybe to put it in other words: In the current API, there is nothing (theoretical or practical) that prevents two different concrete instances of Linker to exist at the same time. There is something that prevents two different instances of ir to exist (in the sense that interface depends on it).

If we want to remove the second restriction (and I do believe this is a desirable goal), we need to make sure the interface does not pretend to have more degrees of freedom than will actually work. So if there are two different concrete instances of Linker depending on two different instances of ir, we need to make sure they do not interact with each other (at the IR level at least).

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.