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

Fixes linter that detects missing .value #4090

Merged
merged 2 commits into from Apr 26, 2018

Conversation

Projects
None yet
3 participants
@eed3si9n
Member

eed3si9n commented Apr 11, 2018

Fixes #4079

#3216 introduced a linter that checks against missing .value, but the tree only checked for Ident. This doesn't work because in reality the symbols of build.sbt are transformed to $somehash.npmInstallTask where somehash is the wrapper object we create. Similarly for the built-in keys, they are presented as sbt.Keys.compile.

With this change unused task will fail to load the build with the following message:

/sbt-4079/build.sbt:26: error: The key `compile` is not being invoked inside the task definition.

Problem: Keys missing `.value` are not initialized and their dependency is not registered.

Solution: Replace `compile` by `compile.value` or remove it if unused.

  compile
  ^
/sbt-4079/build.sbt:27: error: The key `npmInstallTask` is not being invoked inside the task definition.

Problem: Keys missing `.value` are not initialized and their dependency is not registered.

Solution: Replace `npmInstallTask` by `npmInstallTask.value` or remove it if unused.

  npmInstallTask
  ^
Fixes linter that detects missing .value
Fixes #4079

#3216 introduced a linter that checks against missing `.value`, but the tree only checked for `Ident`. This doesn't work because in reality the symbols of build.sbt are transformed to `$somehash.npmInstallTask` where `somehash` is the wrapper object we create. Similarly for the built-in keys, they are presented as `sbt.Keys.compile`.

With this change unused task will fail to load the build with the following message:

```
/sbt-4079/build.sbt:26: error: The key `compile` is not being invoked inside the task definition.

Problem: Keys missing `.value` are not initialized and their dependency is not registered.

Solution: Replace `compile` by `compile.value` or remove it if unused.

  compile
  ^
/sbt-4079/build.sbt:27: error: The key `npmInstallTask` is not being invoked inside the task definition.

Problem: Keys missing `.value` are not initialized and their dependency is not registered.

Solution: Replace `npmInstallTask` by `npmInstallTask.value` or remove it if unused.

  npmInstallTask
  ^
```
@dwijnand

cool.

minor comments, but this can merge as is too

@@ -57,7 +58,7 @@ abstract class BaseTaskLinterDSL extends LinterDSL {
}
@inline def isKey(tpe: Type): Boolean =
tpe <:< taskKeyType || tpe <:< settingKeyType || tpe <:< inputKeyType
tpe <:< initializeType || tpe <:< taskKeyType || tpe <:< settingKeyType || tpe <:< inputKeyType

This comment has been minimized.

@dwijnand

dwijnand Apr 11, 2018

Member

this should simplify to just tpe <:< initializeType because keys are initializes

This comment has been minimized.

@eed3si9n

eed3si9n Apr 11, 2018

Member

Good point. Let me try that.

@@ -66,6 +67,13 @@ abstract class BaseTaskLinterDSL extends LinterDSL {
} else ()
}
def detectAndErrorOnKeyMissingValue(s: Select): Unit = {

This comment has been minimized.

@dwijnand

dwijnand Apr 11, 2018

Member

instead of overloading you could just make the other one take a tree

This comment has been minimized.

@eed3si9n

eed3si9n Apr 11, 2018

Member

This code uses Select#name.

This comment has been minimized.

@eed3si9n

eed3si9n Apr 11, 2018

Member

I guess I can do pattern matching.

This comment has been minimized.

@dwijnand

dwijnand Apr 11, 2018

Member

oh, for some reason I thought all trees had name??

@dwijnand dwijnand added this to the 1.2.0 milestone Apr 11, 2018

@OlegYch

This comment has been minimized.

Contributor

OlegYch commented Apr 11, 2018

does it work for something like (Compile / compile) ?

@dwijnand

This comment was marked as outdated.

Member

dwijnand commented Apr 11, 2018

yes, that's an Initialize too.

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Apr 11, 2018

does it work for something like (Compile / compile) ?

Actually it won't as it stands, because it's only handling Ident and Select. We need Apply for that.

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 25, 2018

Would probably be best for this to not miss the 1.2.0-RC1 train.

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Apr 26, 2018

Added a commit.

@dwijnand

LGTM, pending Travis CI.

@eed3si9n eed3si9n merged commit 851ce6e into sbt:1.x Apr 26, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n deleted the eed3si9n:wip/unused-task branch Apr 26, 2018

@eed3si9n eed3si9n removed the in progress label Apr 26, 2018

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