Illegal dynamic reference: State in dynamic input task

vovapolu commented 7 years ago

steps

Compile this code under sbt 1.0.0

private def testState: Def.Initialize[InputTask[Unit]] =
    Def.inputTaskDyn {
      someFunc(someParser.parsed, state.value)
    }

problem

It throws Illegal dynamic reference: State, but under 0.13.16 sbt everything is alright

expectation

Normal compilation as in sbt 0.13.16

sbt version: 1.0.0

dwijnand commented 7 years ago

Thanks @vovapolu, I'll see if I can identify how this regressed.

fommil commented 6 years ago

@vovapolu this is the bug that's holding back some of the ensime-extras tasks like ensimeCompileOnly, right?

If that is the case, @jvican if you could promote this one it would be awesome. I felt like using ensimeCompileOnly today and remembered it was missing :sob:

vovapolu commented 6 years ago

@fommil ensimeCompileOnly absence is related to changes in compiler api in sbt. This issue affects on ensimeTestOnly task.

fommil commented 6 years ago

do we need changes from sbt to allow that?

vovapolu commented 6 years ago

Allow what? :) We need to rewrite ensimeCompileOnly task for the new api, sbt has already changed this api :) ensimeTestOnly is almost blocked by this issue (I couldn't find a hack to make it work).

fommil commented 6 years ago

Ah ok so the sbt 1.0 is powerful enough to let us to that.

jvican commented 6 years ago

@fommil I cannot promote this one since I'm not hacking on sbt/sbt much these days... Too many projects 😞. What I can provide is a little bit of feedback.

I think that this issue is pretty common when you start using dynamic tasks sparingly, and IMO the "dynamic detection" can be improved. Right now, I believe we're just checking if there's a Select tree (or something that it's not an Ident) within the non-dynamic body, and fail if that's the case. It seems that this check is not good enough, I've hit this limitation before in https://github.com/sbt/sbt/issues/3110. Without looking into the issue, I cannot confidently say this detection is flawed. What I've observed is that it usually fails when the typechecker has applied an implicit conversion (and hence a Select tree), and also in other cases I don't have an hypothesis for.

I think that a solution worth looking into is to whitelist some sbt APIs that we know are safe. For example, selecting a name from an object like sbt.State and sbt.Def (though order of initialization can change here, I wouldn't expect that to affect sbt evaluation semantics).

For now, you can try the workaround I explain in https://github.com/sbt/sbt/issues/3110 and see if ensimeTestOnly compiles.

MasseGuillaume commented 6 years ago

State is a Task, it looks like it's not possible to use any Task in parsed.

olafurpg commented 6 years ago

The error is unrelated to state being a task, the same error happens for settings

// OK, works as expected
run := Def.inputTask {
  val args = sbt.complete.Parsers.spaceDelimited(scalaVersion.value).parsed
  println(args)
}.evaluated

// Error
run := Def.inputTaskDyn {
  val args = sbt.complete.Parsers.spaceDelimited(scalaVersion.value).parsed
                                                 ^^^^^^^^^^^^^^^^^^
                                                 Illegal Dynamic reference
  Def.task(println(args))
}.evaluated

This issue is a blocker for https://github.com/scalacenter/scalafix/issues/844 I would like scalafix <semantic rule> to depend compile.value while scalafix <syntactic rule> should run without compilation. In order to construct the parser I need access to a setting key scalafixDependencies: SettingKey[List[ModuleID]]

olafurpg commented 6 years ago

A workaround I'm using to read global settings from dynamic input tasks is to store them in mutable variables in the plugin object. I set the variable during initialize with

override def globalSettings = List(
  initialize := { myScalaVersion = scalaVersion.value }
)

The parser then accepts a scalaVersion: () => String instead of scalaVersion: String. This may not work for everybody and it would be great to avoid this hacky workaround.

bjaglin commented 4 years ago

Using directly the helpers that the Def.inputTaskDyn macro code would emit if there was no false positives seems to work and to be portable across sbt 0.13.x and sbt 1.x https://github.com/sbt/sbt/blob/b18140c14b1e5fa72e2709576aede414298c3f91/main-settings/src/main/scala/sbt/InputTask.scala#L93-L100

Example of replacing the workaround mentioned just above with that: https://github.com/scalacenter/sbt-scalafix/pull/126/commits/2ca68e4f38cc39d5a7cdd32658e69bcad9d5b6ca#diff-b6db8e3499369917214568aee2dd0960R168

sbt / sbt