Skip to content

REPL - invoke pprint reflectively #24119

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

REPL - invoke pprint reflectively #24119

wants to merge 1 commit into from
+30 −1

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Oct 1, 2025 •
edited
Loading

fixes #24111

reset the print function each time the classloader resets, if cant load reflectively, then fallback to calling directly.

before:

scala -S 3.nightly
Downloading Scala 3.8.0-RC1-bin-20251003-172687a-NIGHTLY compiler
Downloading Scala 3.8.0-RC1-bin-20251003-172687a-NIGHTLY bridge
Welcome to Scala 3.8.0-RC1-bin-20251003-172687a-NIGHTLY-git-172687a (17.0.14, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.
                                                                                                                                 
scala> (1, 2L, 3.0F, "hello", 'a')
val res0: (Int, Long, Float, String, Char) = (1,2,3.0,hello,a)
                                                                                                                                 
scala>

after:

Welcome to Scala 3.8.0-RC1-bin-SNAPSHOT-git-ad52fbe (17.0.14, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.
                                                                                                                                 
scala> (1, 2L, 3.0F, "hello", 'a')
val res0: (Int, Long, Float, String, Char) = (1, 2L, 3.0F, "hello", 'a')
                                                                                                                                 
scala>

@bishabosha
Copy link
Member Author

i think breakage in the interfaces would become clear as repl tests would fail.

But to isolate the classpath issue in a test, perhaps sbt scripted test is appropriate?

@bishabosha bishabosha force-pushed the repl-use-pprint-via-reflection branch from ad52fbe to 549dcbd Compare October 1, 2025 16:08
@bishabosha
Copy link
Member Author

bishabosha commented Oct 1, 2025 •
edited
Loading

another way i guess is compiling a header script with a wrapper print method and invoking that reflectively,

or perhaps there is some way to manipulate the classloaders to avoid reflection?

He-Pin
He-Pin reviewed Oct 1, 2025
View reviewed changes
compiler/src/dotty/tools/repl/Rendering.scala
classOf[Int], // initialOffset
classOf[Boolean], // escape Unicode
classOf[Boolean], // show field names
)
Copy link
Contributor

Choose a reason for hiding this comment

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

with methodHandle？

Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't well suited because method handle is only optimised if its a static final field, but this is based on a dynamic class loader

lihaoyi
lihaoyi reviewed Oct 1, 2025
View reviewed changes
@bishabosha
Copy link
Member Author

bishabosha commented Oct 2, 2025 •
edited
Loading

fails with java.lang.ClassNotFoundException: dotty.shaded.pprint.PPrinter$BlackWhite$ in test-scala3-compiler-bootstrapped and test-scala3-compiler-nonbootstrapped for ReplCompilerTests

2025-10-01T17:11:28.8699192Z [info] Test run dotty.tools.dotc.core.tasty.TastyHeaderUnpicklerTest finished: 0 failed, 0 ignored, 20 total, 0.014s
2025-10-01T17:11:28.8713906Z [info] Test run dotty.tools.repl.ReplCompilerTests started
2025-10-01T17:11:28.8719700Z [info] Test dotty.tools.repl.ReplCompilerTests.testSingletonPrint started
2025-10-01T17:11:29.0459423Z [error] Test dotty.tools.repl.ReplCompilerTests.testSingletonPrint failed: org.junit.ComparisonFailure: expected:<[val a: String = "hello"
2025-10-01T17:11:29.0487191Z [error] val x: a.type = "hello"]> but was:<[java.lang.ClassNotFoundException: dotty.shaded.pprint.PPrinter$BlackWhite$
2025-10-01T17:11:29.0488430Z [error]   at dotty.tools.repl.AbstractFileClassLoader.findClass$$anonfun$1(AbstractFileClassLoader.scala:49)
2025-10-01T17:11:29.0489551Z [error]   at scala.runtime.function.JProcedure1.apply(JProcedure1.java:15)
2025-10-01T17:11:29.0490372Z [error]   at scala.runtime.function.JProcedure1.apply(JProcedure1.java:10)
2025-10-01T17:11:29.0491175Z [error]   at scala.collection.immutable.List.foreach(List.scala:327)
2025-10-01T17:11:29.0492109Z [error]   at dotty.tools.repl.AbstractFileClassLoader.findClass(AbstractFileClassLoader.scala:46)
2025-10-01T17:11:29.0493100Z [error]   at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:592)
2025-10-01T17:11:29.0493908Z [error]   at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
2025-10-01T17:11:29.0495180Z [error]   at dotty.tools.repl.AbstractFileClassLoader.loadClass(AbstractFileClassLoader.scala:60)
2025-10-01T17:11:29.0496144Z [error]   at java.base/java.lang.Class.forName0(Native Method)
2025-10-01T17:11:29.0496825Z [error]   at java.base/java.lang.Class.forName(Class.java:467)
2025-10-01T17:11:29.0497564Z [error]   at dotty.tools.repl.Rendering.pprintRender(Rendering.scala:36)
2025-10-01T17:11:29.0498344Z [error]   at dotty.tools.repl.Rendering.replStringOf(Rendering.scala:86)
2025-10-01T17:11:29.0499132Z [error]   at dotty.tools.repl.Rendering.valueOf$$anonfun$2(Rendering.scala:102)

and again class not found exception in dotty.tools.languageserver.WorksheetTest in test_windows_fast

@lihaoyi
Copy link
Contributor

lihaoyi commented Oct 2, 2025 •
edited
Loading

I guess we can check if the class exists in the classloader, and if not then fall back to the previous implementation?

@bishabosha bishabosha force-pushed the repl-use-pprint-via-reflection branch from 549dcbd to 37a56a6 Compare October 2, 2025 16:45
@bishabosha
Copy link
Member Author

I pushed a change with the fallback option if the class loader doesn't have the class

@bishabosha bishabosha force-pushed the repl-use-pprint-via-reflection branch from 37a56a6 to 350733e Compare October 3, 2025 09:58
@SethTisue SethTisue added this to the 3.8.0 milestone Oct 3, 2025
@bishabosha bishabosha mentioned this pull request Oct 3, 2025
Open
lihaoyi
lihaoyi reviewed Oct 3, 2025
View reviewed changes
natsukagami
natsukagami reviewed Oct 3, 2025
View reviewed changes
@bishabosha
REPL - invoke pprint reflectively
bbbe580 
fixes scala#24111

reflectively load the function each time, (i.e. don't cache). As this is a rare event (i.e user interaction),
any cost is less of a concern.
if cant load reflectively, then fallback to calling directly.
@bishabosha bishabosha force-pushed the repl-use-pprint-via-reflection branch from 350733e to bbbe580 Compare October 6, 2025 15:31
@bishabosha
Copy link
Member Author

now with no caching - meaning there is no complex state management anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natsukagami natsukagami natsukagami left review comments

+2 more reviewers

@He-Pin He-Pin He-Pin left review comments

@lihaoyi lihaoyi lihaoyi left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.

pprint not destructuring results in REPL
5 participants
@bishabosha @lihaoyi @He-Pin @natsukagami @SethTisue