Fixes for sbt-dotty #2690

smarter · 2017-06-06T09:06:40Z

No description provided.

In the next commit we'll extend configureIDE to work even when it needs to update some settings to set the scalaVersion to Dotty. Since this update takes some time and is discarded at the end of the task, it makes sense to run the configuration and compilation steps in the same task to avoid having to do the settings update twice.

sjrd · 2017-06-06T12:42:30Z

sbt-dotty/src/dotty/tools/sbtplugin/DottyIDEPlugin.scala

+        // Not needed to generate the config, but this guarantees that the
+        // generated config is usable by an IDE without any extra compilation
+        // step.
+        val _ = compile.value


This is different from what prepareIDE did before. It called compileForIDE.value, not compile.value.

Right, but all compileForIDE does is call compile on each dotty configuration, here we already are in a task run in every dotty configuration, so compile is fine.

OK but then, why is compileForIDE still necessary? It doesn't seem to be called by anything.

sjrd · 2017-06-06T12:48:31Z

sbt-dotty/src/dotty/tools/sbtplugin/DottyIDEPlugin.scala

+        if (isDottyVersion(version))
+          Some((version, projRef))
+        else
+          crossScalaVersions.in(projRef).get(settings).get.filter(isDottyVersion).sorted.lastOption match {


I very much doubt sorted sorts in a meaningful way here.

Yeah I had a FIXME before that we need a real ordering for version numbers, will add it back.

sjrd · 2017-06-06T12:53:51Z

sbt-dotty/src/dotty/tools/sbtplugin/DottyIDEPlugin.scala

+    if (dottyVersions.isEmpty)
+      throw new FeedbackProvidedException {
+        override def toString = "No Dotty project detected"
+      }


Throwing a FeedbackProvidedException seems weird. I wouldn't expect that to be thrown if there isn't at least some error logging before that, otherwise the user won't see anything, right?

Consider throw new MessageOnlyException("No Dotty project detected") instead.

Hmm, I thought FeedbackProvidedException displayed its toString in sbt? Will check.

MessageOnlyException is the "standard" one if you want to display a message and fail the task, but not display the stack trace of the exception.

sjrd · 2017-06-06T12:54:08Z

sbt-dotty/src/dotty/tools/sbtplugin/DottyIDEPlugin.scala

+        override def toString = "No Dotty project detected"
+      }
+    else {
+      val dottyVersion = dottyVersions.sorted.last


Again, sorted probably does not do what you want.

sjrd · 2017-06-06T13:01:30Z

sbt-dotty/src/dotty/tools/sbtplugin/DottyIDEPlugin.scala

-        // If different versions of Dotty are used by subprojects, choose the latest one
-        // FIXME: use a proper version number Ordering that knows that "0.1.1-M1" < "0.1.1"
-        val ideVersion = configs.map(_.compilerVersion).sorted.last
+    configureIDE := Def.taskDyn {


configureIDE should not be a Task anymore, if it's going to play with States, create new ones and run tasks within those modified States. It should definitely be a Command. Otherwise you expose yourself to race conditions if configureIDE is run in parallel with other tasks.

sjrd · 2017-06-06T13:03:28Z

sbt-dotty/src/dotty/tools/sbtplugin/DottyIDEPlugin.scala

-        // FIXME: use a proper version number Ordering that knows that "0.1.1-M1" < "0.1.1"
-        val ideVersion = configs.map(_.compilerVersion).sorted.last
+    configureIDE := Def.taskDyn {
+      val origState = state.value


I expect this hackery that doesn't make sense to me not to be necessary with a Command.

sjrd · 2017-06-06T13:07:09Z

sbt-dotty/src/dotty/tools/sbtplugin/DottyIDEPlugin.scala

+        val (dottyVersion, projRefs, dottyState) = dottySetup(origState)
+        val configs0 = runInAllConfigurations(projectConfig, projRefs, dottyState).flatten
+
+        // Drop configurations who do not define their own sources, but just


typo: who -> that

sjrd · 2017-06-06T13:08:30Z

sbt-dotty/src/dotty/tools/sbtplugin/DottyIDEPlugin.scala

        val pwArtifact = new PrintWriter(".dotty-ide-artifact")
-        pwArtifact.println(s"ch.epfl.lamp:dotty-language-server_0.1:${ideVersion}")
+        pwArtifact.println(s"ch.epfl.lamp:dotty-language-server_${dlsBinaryVersion}:${dlsVersion}")
        pwArtifact.close()


er ... try/finally for the close()?

pwArtifact.println does not throw exception, so why would a try/finally help here?

Idk, visceral reaction to seeing an allocation of a file stream and its close() without try/finally, I guess 😄. Right now it might not make a difference, but who knows what will happen in the future with that code, and someone will introduce a potentially-exception-throwing code and forget to add the try/finally.

Alright, will add. :)

sjrd · 2017-06-06T13:12:07Z

sbt-dotty/src/dotty/tools/sbtplugin/DottyIDEPlugin.scala

+        runInAllConfigurations(compile, projRefs, dottyState)
+        ()
+      }
+    }.value,


This is changed in this commit, but it appears not to be used whatsoever, due to the comment I made on the previous commit.

It's not used, but can be manually called by the user, e.g. you may want to run sbt ~compileForIDE in the background of the IDE. This is undocumented because it's likely to change.

So far, configureIDE detected the dotty-compiled projects by looking at `scalaVersion`, with this commit we now also look at `crossScalaVersions`. This is considerably harder to do since extracting the settings requires updating `scalaVersion` in all projects where it's not set to a dotty version but `crossScalaVersion` is, moreover this update needs to be temporary: once the `configureIDE` task is done, the state should revert to the original one.

This makes sense since they deal with the state (even if they always return the original state).

smarter · 2017-06-06T21:54:57Z

PR updated, thanks for the comments!

sjrd · 2017-06-07T13:25:47Z

👍

olafurpg · 2017-06-07T13:26:18Z

🎉 any chance to get a new sbt-dotty release out?

smarter · 2017-06-07T13:32:24Z

Only @felixmulder has the magic powers to do the release, he told me he would do it at 6 PM :).

felixmulder · 2017-06-07T17:11:42Z

The CI should do it henceforth: #2706
🎉

PS - I'm the only one with magic powers, because nobody else wants the powers XD

smarter mentioned this pull request Jun 6, 2017

Update sbt-dotty lampepfl/sbt-dotty#10

Closed

smarter requested a review from sjrd June 6, 2017 09:15

sjrd requested changes Jun 6, 2017

View reviewed changes

olafurpg mentioned this pull request Jun 6, 2017

Upgrade to 0.2.x nightly builds lampepfl/compiler-benchmark#13

Open

smarter added 6 commits June 6, 2017 23:19

Update dottyLatestNightlyBuild to fetch dotty 0.2

ba33872

Use MessageOnlyException instead of FeedbackProvidedException

f4960d1

Be more paranoid about closing files

34b04d4

configureIDE and compileForIDE are now commands instead of tasks

bd1df68

This makes sense since they deal with the state (even if they always return the original state).

sbt-dotty: Bump version to 0.1.2

4d05747

smarter force-pushed the ide-cross branch from 864edaf to 4d05747 Compare June 6, 2017 21:54

olafurpg mentioned this pull request Jun 7, 2017

Faster base class sets #2676

Merged

smarter merged commit 6f8bc43 into scala:master Jun 7, 2017

allanrenucci deleted the ide-cross branch December 14, 2017 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for sbt-dotty #2690

Fixes for sbt-dotty #2690

smarter commented Jun 6, 2017

sjrd Jun 6, 2017

smarter Jun 6, 2017

sjrd Jun 6, 2017

sjrd Jun 6, 2017

smarter Jun 6, 2017

sjrd Jun 6, 2017

smarter Jun 6, 2017

sjrd Jun 6, 2017

sjrd Jun 6, 2017

sjrd Jun 6, 2017

sjrd Jun 6, 2017

sjrd Jun 6, 2017

sjrd Jun 6, 2017

smarter Jun 6, 2017

sjrd Jun 6, 2017 •

edited

Loading

smarter Jun 6, 2017

sjrd Jun 6, 2017

smarter Jun 6, 2017

sjrd Jun 6, 2017

smarter commented Jun 6, 2017

sjrd commented Jun 7, 2017

olafurpg commented Jun 7, 2017

smarter commented Jun 7, 2017

felixmulder commented Jun 7, 2017 •

edited

Loading

Fixes for sbt-dotty #2690

Fixes for sbt-dotty #2690

Conversation

smarter commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarter commented Jun 6, 2017

sjrd commented Jun 7, 2017

olafurpg commented Jun 7, 2017

smarter commented Jun 7, 2017

felixmulder commented Jun 7, 2017 • edited Loading

sjrd Jun 6, 2017 •

edited

Loading

felixmulder commented Jun 7, 2017 •

edited

Loading