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 thunk for slash syntax #3620

Merged
merged 14 commits into from Oct 7, 2017

Conversation

Projects
None yet
2 participants
@eed3si9n
Member

eed3si9n commented Oct 6, 2017

Ref #3606, #3611, and #3613

This removes unnecessary thunk for slash syntax.
The semantics using this approach is strictly better than the previous in (ref, config, task). By removing the thunk, we retain (a / b) / c == a / b / c.

See the following example:

scala> import sbt._, Keys._
scala> val t: TaskKey[Unit] = (test in Test)
t: sbt.TaskKey[Unit] = TaskKey(This / Select(ConfigKey(test)) / This / test)

scala> ThisBuild / t
ThisBuild / t
res1: sbt.TaskKey[Unit] = TaskKey(Select(ThisBuild) / Select(ConfigKey(test)) / This / test)

scala> ThisBuild / t / name
ThisBuild / t / name
res2: sbt.SettingKey[String] = SettingKey(Select(ThisBuild) / Select(ConfigKey(test)) / Select(test) / name)

so far so good? Now look at this:

scala> name in (ThisBuild, t)
name in (ThisBuild, t)
res3: sbt.SettingKey[String] = SettingKey(Select(ThisBuild) / This / Select(test) / name)

Test configuration knowledge is lost! For in (..) maybe it was ok because mostly we don't use unscoped keys, but that's the difference between in (..) and /.

Fixes #3605

Remove thunk for slash syntax
Ref #3606, #3611, and #3613

This removes unnecessary thunk for slash syntax.
The semantics using this approach is strictly better than the previous `in (ref, config, task)`. By removing the thunk, we retain `(a / b) / c == a / b / c`.

See the following example:

```scala
scala> import sbt._, Keys._
scala> val t: TaskKey[Unit] = (test in Test)
t: sbt.TaskKey[Unit] = TaskKey(This / Select(ConfigKey(test)) / This / test)

scala> ThisBuild / t
ThisBuild / t
res1: sbt.TaskKey[Unit] = TaskKey(Select(ThisBuild) / Select(ConfigKey(test)) / This / test)

scala> ThisBuild / t / name
ThisBuild / t / name
res2: sbt.SettingKey[String] = SettingKey(Select(ThisBuild) / Select(ConfigKey(test)) / Select(test) / name)
```

so far so good? Now look at this:

```
scala> scala> name in (ThisBuild, t)
name in (ThisBuild, t)
res3: sbt.SettingKey[String] = SettingKey(Select(ThisBuild) / This / Select(test) / name)
```

`Test` configuration knowledge is lost! For `in (..)` maybe it was ok because mostly we don't use unscoped keys, but that's the difference between `in (..)` and `/`.

Fixes #3605
@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Oct 6, 2017

Member
scala> ThisBuild / t / name
ThisBuild / t / name
res2: sbt.SettingKey[String] = SettingKey(Select(ThisBuild) / Select(ConfigKey(test)) / Select(test) / name)

This means: define the name key, in ThisBuild scope and in t task-scope. It shouldn't be using t configuration scope.

Member

dwijnand commented Oct 6, 2017

scala> ThisBuild / t / name
ThisBuild / t / name
res2: sbt.SettingKey[String] = SettingKey(Select(ThisBuild) / Select(ConfigKey(test)) / Select(test) / name)

This means: define the name key, in ThisBuild scope and in t task-scope. It shouldn't be using t configuration scope.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Oct 6, 2017

Member

If t it scoped to Test it makes sense to respect t's existing scope. "To scope" means to override with non-This.

Member

eed3si9n commented Oct 6, 2017

If t it scoped to Test it makes sense to respect t's existing scope. "To scope" means to override with non-This.

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Oct 6, 2017

Member

When t is used in task-scope position it should only be setting the task-scope axis.

Member

dwijnand commented Oct 6, 2017

When t is used in task-scope position it should only be setting the task-scope axis.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Oct 6, 2017

Member

When t is used in task-scope position it should only be setting the task-scope axis.

That's not referentially transparent. Here's another example:

scala> val t: TaskKey[Unit] = Test / test
val t: TaskKey[Unit] = Test / test
t: sbt.TaskKey[Unit] = TaskKey(This / Select(ConfigKey(test)) / This / test)

scala> t / streams
t / streams
res2: sbt.TaskKey[sbt.Keys.TaskStreams] = TaskKey(This / Select(ConfigKey(test)) / Select(test) / streams)

This is going to be identical to when you replace t:

scala> Test / test / streams
Test / test / streams
res3: sbt.TaskKey[sbt.Keys.TaskStreams] = TaskKey(This / Select(ConfigKey(test)) / Select(test) / streams

For 99% of the times, this will not show up since most people will use just the unscoped TaskKey, but this implementation is an improvement over treating t as just a holder for an attribute key.

Member

eed3si9n commented Oct 6, 2017

When t is used in task-scope position it should only be setting the task-scope axis.

That's not referentially transparent. Here's another example:

scala> val t: TaskKey[Unit] = Test / test
val t: TaskKey[Unit] = Test / test
t: sbt.TaskKey[Unit] = TaskKey(This / Select(ConfigKey(test)) / This / test)

scala> t / streams
t / streams
res2: sbt.TaskKey[sbt.Keys.TaskStreams] = TaskKey(This / Select(ConfigKey(test)) / Select(test) / streams)

This is going to be identical to when you replace t:

scala> Test / test / streams
Test / test / streams
res3: sbt.TaskKey[sbt.Keys.TaskStreams] = TaskKey(This / Select(ConfigKey(test)) / Select(test) / streams

For 99% of the times, this will not show up since most people will use just the unscoped TaskKey, but this implementation is an improvement over treating t as just a holder for an attribute key.

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Oct 7, 2017

Member

It is referentially transparent:

scala> val t = Test / test
val t = Test / test
t: sbt.SlashSyntax.ScopeAndTaskKey[Unit] = This / Select(ConfigKey(test)) / This / test

scala> t / streams
t / streams
res0: sbt.TaskKey[sbt.Keys.TaskStreams] = TaskKey(This / Select(ConfigKey(test)) / Select(test) / streams)

scala> Test / test / streams
Test / test / streams
res1: sbt.TaskKey[sbt.Keys.TaskStreams] = TaskKey(This / Select(ConfigKey(test)) / Select(test) / streams)
Member

dwijnand commented Oct 7, 2017

It is referentially transparent:

scala> val t = Test / test
val t = Test / test
t: sbt.SlashSyntax.ScopeAndTaskKey[Unit] = This / Select(ConfigKey(test)) / This / test

scala> t / streams
t / streams
res0: sbt.TaskKey[sbt.Keys.TaskStreams] = TaskKey(This / Select(ConfigKey(test)) / Select(test) / streams)

scala> Test / test / streams
Test / test / streams
res1: sbt.TaskKey[sbt.Keys.TaskStreams] = TaskKey(This / Select(ConfigKey(test)) / Select(test) / streams)

dwijnand added some commits Oct 7, 2017

@dwijnand

The referential transparency/implicit conversion is concerning. Not having semantic parity "in" syntax is also concerning. I've submitted eed3si9n#3 that adds slash support for "bare" AttributeKeys, which have parity with "in". I think that covers all concerns. At some point I'd like to add some negative tests to check that you can't, for example, call Global / Compile / scalacOption.

eed3si9n and others added some commits Oct 7, 2017

Merge pull request #3 from dwijnand/wip/previous4
Add slash syntax for AttrKey that parity with "in" syntax
@dwijnand

Some minor fixes in eed3si9n#4. But this LGTM to merge.

@eed3si9n eed3si9n merged commit 84dafd0 into sbt:1.x Oct 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n deleted the eed3si9n:wip/previous4 branch Oct 7, 2017

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