Skip to content

Conversation

aherlihy
Copy link
Contributor

@aherlihy aherlihy marked this pull request as draft January 10, 2025 20:55
}
}

// TODO: no idea how to access and reload the class paths
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty tricky. There is a method updateClassPath but it's not used anywhere and I'm pretty sure it never worked and was just copy-pasted from Scala 2.

ctx.platform.classPath is used from ctx.platform.rootLoader which is itself used to setup RootClass which corresponds to __root__ from which all imports start. But RootClass is a symbol created once when we initialize the initial context and never again, so updateClassPath won't actually help us import any new symbol

So I think we need to recreate a rootCtx, which is akin to what :settings seems to do:

rootCtx = setupRootCtx(tokenize(arg).toArray, rootCtx)
state.copy(context = rootCtx)
but I don't guarantee this works 😅

@Gedochao Gedochao added the release-notes Should be mentioned in the release notes label Jan 13, 2025
@dwijnand
Copy link
Member

Had to rebase to integrate with the :kind changes.

@dwijnand dwijnand marked this pull request as ready for review February 17, 2025 11:43
@dwijnand dwijnand requested a review from smarter February 17, 2025 11:43
@aherlihy aherlihy changed the title Implement :require [WIP] Implement :require Feb 26, 2025
@aherlihy aherlihy changed the title Implement :require Implement :jar (deprecate :require) Feb 26, 2025
@aherlihy
Copy link
Contributor Author

Per the discussion on #21658 rename :require to :jar and add a deprecation message to :require.

Copy link
Contributor

@bracevac bracevac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Had some questions.

aherlihy and others added 2 commits March 4, 2025 12:54
Co-authored-by: Oliver Bračevac <bracevac@users.noreply.github.com>
@aherlihy aherlihy enabled auto-merge March 6, 2025 13:10
@Gedochao Gedochao requested a review from bracevac March 7, 2025 15:07
@Gedochao Gedochao linked an issue Mar 7, 2025 that may be closed by this pull request
Copy link
Contributor

@bracevac bracevac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked and I think the exception handling needs to be more robust. For example, if you feed it a file that is not a jar, it will crash the entire REPL session:

scala> :jar fakejar.jar
Exception in thread "main" java.io.IOException: Error accessing fakejar.jar
	at dotty.tools.io.FileZipArchive.dotty$tools$io$FileZipArchive$$openZipFile(ZipArchive.scala:126)
	at dotty.tools.io.FileZipArchive.$1$$lzyINIT1(ZipArchive.scala:164)
	at dotty.tools.io.FileZipArchive.$1$(ZipArchive.scala:161)
	at dotty.tools.io.FileZipArchive.root(ZipArchive.scala:161)
	at dotty.tools.io.FileZipArchive.iterator(ZipArchive.scala:200)
	at dotty.tools.repl.ReplDriver.flatten$1(ReplDriver.scala:527)
	at dotty.tools.repl.ReplDriver.interpretCommand(ReplDriver.scala:530)
	at dotty.tools.repl.ReplDriver.interpret(ReplDriver.scala:308)
	at dotty.tools.repl.ReplDriver.loop$1(ReplDriver.scala:219)
	at dotty.tools.repl.ReplDriver.runUntilQuit$$anonfun$1(ReplDriver.scala:222)
	at dotty.tools.repl.ReplDriver.withRedirectedOutput(ReplDriver.scala:256)
	at dotty.tools.repl.ReplDriver.runBody$$anonfun$1(ReplDriver.scala:230)
	at dotty.tools.runner.ScalaClassLoader$.asContext(ScalaClassLoader.scala:80)
	at dotty.tools.repl.ReplDriver.runBody(ReplDriver.scala:230)
	at dotty.tools.repl.ReplDriver.runUntilQuit(ReplDriver.scala:222)
	at dotty.tools.repl.ReplDriver.tryRunning(ReplDriver.scala:159)
	at dotty.tools.repl.Main$.main(Main.scala:7)
	at dotty.tools.repl.Main.main(Main.scala)
Caused by: java.util.zip.ZipException: zip file is empty
	at java.base/java.util.zip.ZipFile$Source.zerror(ZipFile.java:1769)
	at java.base/java.util.zip.ZipFile$Source.findEND(ZipFile.java:1553)
	at java.base/java.util.zip.ZipFile$Source.initCEN(ZipFile.java:1648)
	at java.base/java.util.zip.ZipFile$Source.<init>(ZipFile.java:1486)
	at java.base/java.util.zip.ZipFile$Source.get(ZipFile.java:1448)
	at java.base/java.util.zip.ZipFile$CleanableResource.<init>(ZipFile.java:718)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:252)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:181)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:195)
	at dotty.tools.io.FileZipArchive.dotty$tools$io$FileZipArchive$$openZipFile(ZipArchive.scala:123)
	... 17 more

Here, fakejar.jar is an empty file on the filesystem.
Ideally, this should not crash and we can continue using the REPL.

@Gedochao
Copy link
Contributor

Gedochao commented Mar 7, 2025

Perhaps we should MIME check that it is indeed a JAR file?

@bracevac
Copy link
Contributor

bracevac commented Mar 7, 2025

Perhaps we should MIME check that it is indeed a JAR file?

This doesn't really solve the issue. I further tried two more things:

  1. Create a self-signed jar file and append random data to its end. This will be happily accepted by the :jar command. java rejects it. General question here is should we check signatures?

  2. Create a jar from a corrupted classfile. This will also lead to a complete REPL crash.

scala> :jar corrupt/HelloWorld.jar
Exception in thread "main" java.lang.IllegalArgumentException
	at scala.tools.asm.ClassReader.<init>(ClassReader.java:263)
	at scala.tools.asm.ClassReader.<init>(ClassReader.java:180)
	at scala.tools.asm.ClassReader.<init>(ClassReader.java:166)
	at scala.tools.asm.ClassReader.<init>(ClassReader.java:288)
	at dotty.tools.repl.ReplDriver.tryClassLoad$1(ReplDriver.scala:535)
	at dotty.tools.repl.ReplDriver.$anonfun$16(ReplDriver.scala:546)
	at scala.collection.IterableOnceOps.find(IterableOnce.scala:678)
	at scala.collection.IterableOnceOps.find$(IterableOnce.scala:674)
	at scala.collection.AbstractIterator.find(Iterator.scala:1306)
	at dotty.tools.repl.ReplDriver.interpretCommand(ReplDriver.scala:546)
	at dotty.tools.repl.ReplDriver.interpret(ReplDriver.scala:308)
	at dotty.tools.repl.ReplDriver.loop$1(ReplDriver.scala:219)
	at dotty.tools.repl.ReplDriver.runUntilQuit$$anonfun$1(ReplDriver.scala:222)
	at dotty.tools.repl.ReplDriver.withRedirectedOutput(ReplDriver.scala:256)
	at dotty.tools.repl.ReplDriver.runBody$$anonfun$1(ReplDriver.scala:230)
	at dotty.tools.runner.ScalaClassLoader$.asContext(ScalaClassLoader.scala:80)
	at dotty.tools.repl.ReplDriver.runBody(ReplDriver.scala:230)
	at dotty.tools.repl.ReplDriver.runUntilQuit(ReplDriver.scala:222)
	at dotty.tools.repl.ReplDriver.tryRunning(ReplDriver.scala:159)
	at dotty.tools.repl.Main$.main(Main.scala:7)
	at dotty.tools.repl.Main.main(Main.scala)

I think at the very least, we should have somewhere a catch-all to prevent exceptions from crashing the REPL.

About signatures, I guess we should allow unsigned jars. But do we want to optionally check signatures? That could be addressed in a follow-up issue after the cutoff.

@Gedochao
Copy link
Contributor

About signatures, I guess we should allow unsigned jars. But do we want to optionally check signatures? That could be addressed in a follow-up issue after the cutoff.

should plan that as a follow-up issue IMO

I think at the very least, we should have somewhere a catch-all to prevent exceptions from crashing the REPL.

We can have a catch-all to stop the REPL from crashing, but still perform checks (like MIME) to properly warn where the problem is.

@bracevac
Copy link
Contributor

In the interest of making it before the cutoff, I suggest to just implement the catch-all now and look into MIME and signatures afterwards.

Co-authored-by: Piotr Chabelski <ged.subfan@gmail.com>
@aherlihy
Copy link
Contributor Author

Added a catch-all around :jar for now.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only could do a superficial review and am relying on a deeper review by others.
From what I could see, the code looks good to me,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for the :require command in the REPL
7 participants