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

Remove differences in diagnostic printing in the REPL #13266

Merged
merged 2 commits into from Sep 1, 2021

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Aug 7, 2021

No description provided.

@dwijnand dwijnand marked this pull request as ready for review August 7, 2021 19:33
@dwijnand
Copy link
Member Author

Who's the closest to being the REPL maintainer?

@SethTisue
Copy link
Member

can you explain the build.sbt changes a little?

@dwijnand
Copy link
Member Author

can you explain the build.sbt changes a little?

+  val repl = taskKey[Unit]("spawns a repl with the correct classpath") // (1)
..
-    // Spawns a repl with the correct classpath
-    addCommandAlias("repl", "scala3-compiler-bootstrapped/console") // (2)
..
+    repl := (Compile / console).value,        // (3)
+    Compile / console / scalacOptions := Nil, // (4)
..
+        repl := (`scala3-compiler-bootstrapped` / repl).value, // (5)

Sure.

(1), (2), and (3): I was thinking (2) did the double-compilation of bootstrapping and then building a bootstrapped compiler for the REPL, so I was looking to copy how scala and scalac are set up, as tasks, so you can run either scala3-compiler/scalac or scala3-compiler-bootstrapped/scalac. But scala3-compiler/repl didn't work as desired, because it's based on the project's scalaVersion. So I just left the converting to a task: (1) and (3).

(5): But then the root project doesn't aggregate scala3-compiler-bootstrapped, so I need to manually set up that delegation.

(4): Finally, I don't want the scalacOptions used in compiling the (bootstrapped) compiler from tainting the out-the-box experience of using the REPL, e.g. by enabling -deprecated and -feature, so I reset the scalacOptions.

@dwijnand
Copy link
Member Author

Rebased because this conflicted with #13000. Notice from the output diff how this PR removes any remaining differences.

Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

The user-facing changes look very good to me. This is a nice improvement in usability.

I'm satisfied by the build.sbt explanation (to the extent that I understand the dotty build, anyway).

And, Dale walked me through the implementation just now and I see nothing to object to. A few more comments might help future maintainers.

@bjornregnell any thoughts?

@som-snytt
Copy link
Contributor

Footnote that Scala 2 REPL enables -deprecation -feature unless those flags are explicitly set. (Actually in MainGenericRunner, the test is now that Wconf and Xlint are not set.) You only see extended warning once:

scala
Welcome to Scala 2.13.6 (OpenJDK 64-Bit Server VM, Java 17-ea).
Type in expressions for evaluation. Or try :help.

scala> implicit def f(s: String) = s.toDouble
                    ^
       warning: implicit conversion method f should be enabled
       by making the implicit value scala.language.implicitConversions visible.
       This can be achieved by adding the import clause 'import scala.language.implicitConversions'
       or by setting the compiler option -language:implicitConversions.
       See the Scaladoc for value scala.language.implicitConversions for a discussion
       why the feature should be explicitly enabled.
def f(s: String): Double

scala> implicit def f(s: String) = s.toDouble
                    ^
       warning: implicit conversion method f should be enabled
       by making the implicit value scala.language.implicitConversions visible.
def f(s: String): Double

scala>

@SethTisue
Copy link
Member

SethTisue commented Sep 1, 2021

@som-snytt True, but regardless, the sbt task should have the same defaults as what users get. I would be in favor of a separate PR that added -deprecation -feature in both contexts.

For example:

    scala> @deprecated("no", "nah") class C; new C
    there were 1 deprecation warning(s); re-run with -deprecation for details
    // defined class C
    val res0: C = C@4273cda6
@dwijnand dwijnand merged commit 13250bc into scala:master Sep 1, 2021
@nicolasstucki
Copy link
Contributor

@dwijnand the CI failed on the merge commit if this PR. It then failed on nightly.

@dwijnand dwijnand deleted the repl-diagnostic-print branch September 2, 2021 06:16
@dwijnand
Copy link
Member Author

dwijnand commented Sep 2, 2021

Does GitHub Actions run on the branch then, rather than a merge commit? I'll fix up master asap.

@Kordyjan Kordyjan added this to the 3.1.1 milestone Aug 2, 2023
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

Successfully merging this pull request may close these issues.

None yet

5 participants