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
Merged
merged 1 commit into from
Oct 13, 2025
Merged

REPL - invoke pprint reflectively #24119

merged 1 commit into from
Oct 13, 2025

Conversation

@bishabosha
Copy link
Member

@bishabosha bishabosha commented Oct 1, 2025 •
edited
Loading

fixes #24111

load print function each time it is called, 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 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

@Gedochao Gedochao requested review from bracevac and tgodzik October 8, 2025 08:59
@bishabosha bishabosha force-pushed the repl-use-pprint-via-reflection branch from bbbe580 to 0a7b3b2 Compare October 8, 2025 12:06
@bishabosha
REPL - invoke pprint reflectively
849b21a 
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 0a7b3b2 to 849b21a Compare October 8, 2025 12:08
@bishabosha
Copy link
Member Author

updated to include the leading offset characters from #24118

natsukagami
natsukagami approved these changes Oct 8, 2025
View reviewed changes
Copy link
Contributor

@natsukagami natsukagami left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@tgodzik tgodzik requested a review from lihaoyi October 13, 2025 11:21
tgodzik
tgodzik approved these changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Looks good! @lihaoyi anything from your side?

@lihaoyi
Copy link
Contributor

lihaoyi commented Oct 13, 2025

@tgodzik nope im all good

@tgodzik tgodzik merged commit 3ec5383 into scala:main Oct 13, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgodzik tgodzik tgodzik approved these changes

@natsukagami natsukagami natsukagami approved these changes

@bracevac bracevac Awaiting requested review from bracevac

@lihaoyi lihaoyi Awaiting requested review from lihaoyi

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

Assignees

@tgodzik tgodzik

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

6 participants

@bishabosha @lihaoyi @He-Pin @tgodzik @natsukagami @SethTisue