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

[sbt 1.0] Add first round of DSL checks to sbt #3216

Merged
merged 12 commits into from May 25, 2017

Conversation

Projects
None yet
5 participants
@jvican
Copy link
Member

commented May 25, 2017

This pull request adds the necessary infrastructure to run sbt checks. The
purpose of the PR is to allow sbt to prohibit DSL usages that are dangerous and
yield unexpected results. One of these cases is the use of value inside if
expressions (though there are more).

This will make it easier both to learn sbt and to use it. I am very optimistic
about the result of these changes in the close future.

For the review, I suggest having a look at the independent commits to get an
idea of what I am doing in every one of them.

jvican added some commits May 24, 2017

Replace previous scalafmt plugin by neo-scalafmt
The previous scalafmt plugin had two problems:

* Caching with Coursier did not work correctly
* It failed after the upgrade to 1.0 in my computer (from a clean fork):

```
[error] (testingProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (runProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (taskProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (stdTaskProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (actionsProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (protocolProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (commandProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (mainSettingsProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (mainProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (sbtProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (scriptedPluginProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] (scriptedSbtProj/compile:scalafmtInc) java.lang.NoClassDefFoundError: Could not initialize class scala.meta.package$
[error] Total time: 19 s, completed May 24, 2017 10:44:56 AM
```

This commit replaces the previous scalafmt integration by the one
created by Lucidsoftware, big shoutout! (/cc @pauldraper)

https://github.com/lucidsoftware/neo-sbt-scalafmt
Add check of task invocation inside `if`
This commit adds the first version of the checker that will tell users
if they are doing something wrong.

The first version warns any user that write `.value` inside an if
expression.

As the test integration is not yet working correctly and messages are
swallowed, we have to println to get info from the test.
@jvican

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

The error messages are inspired by those of Rust and those of Dotty.

In the future, for sbt DSL checks that are warnings and not errors, I'd like to add a showError $ERROR_ID where $ERROR_ID corresponds to an identifier shown in the message that elaborates on the reported error, the consequences, et cetera.

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

An example of the sbt check:
sbt-dsl-error-if

jvican added some commits May 25, 2017

Add fully-fledged macro check for value inside if
`.value` inside the if of a regular task is unsafe. The wrapping task
will always execute the value, no matter what the if predicate yields.

This commit adds the infrastructure to lint code for every sbt DSL
macro. It also adds example of neg tests that check that the DSL checks
are in place.

The sbt checks yield error for this specific case because we may want to
explore changing this behaviour in the future. The solutions to this are
straightforward and explained in the error message, that looks like
this:

```
EXPECTED: The evaluation of `fooNeg` happens always inside a regular task.

PROBLEM: `fooNeg` is inside the if expression of a regular task.
  Regular tasks always evaluate task inside the bodies of if expressions.

SOLUTION:
  1. If you only want to evaluate it when the if predicate is true, use a dynamic task.
  2. Otherwise, make the static evaluation explicit by evaluating `fooNeg` outside the if expression.
```

Aside from those solutions, this commit also adds a way to disable any
DSL check by using the new `sbt.unchecked` annotation. This annotation,
similar to `scala.annotation.unchecked` disables compiler output. In our
case, it will disable any task dsl check, making it silent.

Examples of positive checks have also been added.

There have been only two places in `Defaults.scala` where this check has
made compilation fail.

The first one is inside `allDependencies`. To ensure that we still have
static dependencies for `allDependencies`, I have hoisted up the value
invocation outside the if expression. We may want to explore adding a
dynamic task in the future, though. We are doing unnecessary work there.

The second one is inside `update` and is not important because it's not
exposed to the user. We use a `taskDyn`.

@jvican jvican force-pushed the scalacenter:macro-usability-improvements branch 3 times, most recently from 0d14d75 to 25994ef May 25, 2017

Improve if check and prohibit value inside anon
This commit does the following things:

* Removes the boolean from the instance context passes to the linter.
* Prohibits the use of value inside anonymous functions.
* Improves the previous check of `value` inside if.

The improvements have occurred thanks to the fix of an oversight in the
traverser. As a result, several implementation of tasks have been
rewritten because of new compilation failures by both checks.

Note that the new check that prohibits the use of value inside anonymous
functions ignores all the functions whose parameters have been
synthesized by scalac (that can happen in a number of different
scenarios, like for comprehensions). Other scripted tests have also been
fixed.

Running `.value` inside an anonymous function yields the following
error:

```
[error] /data/rw/code/scala/sbt/main-settings/src/test/scala/sbt/std/TaskPosSpec.scala:50:24: The evaluation of `foo` inside an anonymous function is prohibited.
[error]
[error] Problem: Task invocations inside anonymous functions are evaluated independently of whether the anonymous function is invoked or not.
[error]
[error] Solution:
[error]   1. Make `foo` evaluation explicit outside of the function body if you don't care about its evaluation.
[error]   2. Use a dynamic task to evaluate `foo` and pass that value as a parameter to an anonymous function.
[error]
[error]       val anon = () => foo.value + " "
[error]                        ^
```

@jvican jvican force-pushed the scalacenter:macro-usability-improvements branch from 25994ef to af562dc May 25, 2017

@eed3si9n
Copy link
Member

left a comment

The change looks nice!

I'm just not too sure about the unchecked stuff.

*
* @since 1.0.0
*/
class unchecked extends Annotation

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n May 25, 2017

Member

I don't know if it's a good idea to reintroduce a famous annotation like this. Could we just use the real unchecked for this purpose?

This comment has been minimized.

Copy link
@jvican

jvican May 25, 2017

Author Member

The reason I preferred to create our own unchecked annotation is because reusing the Scala one can cause more confusion, and it's used for different purposes so I don't want to overload its semantics. The good thing is that scala.annotation.unchecked is by default not in the scope of sbt files, but sbt.unchecked is -- so you can have a seamless experience. Plugin files will need to import explicitly.

I think that keeping the same name as the Scala one is good for people to remember it. They share the name, but not the namespace. Otherwise, it's a tedious task to remember "that special annotation that disables the sbt checks".

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n May 25, 2017

Member

Plugin authors regularly write import sbt._. So I'd be surprised if there are ambiguity of "unchecked" if I tried to use the scala.annotation.unchecked. I think we should a new name like "lintoff" since the meaning is also slightly different.

This comment has been minimized.

Copy link
@jvican

jvican May 25, 2017

Author Member

Good argument. lintoff sounds a little bit unclear though. What about @sbtUnchecked? I would like to reuse the same meaning as scala.annotation.unchecked.

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n May 25, 2017

Member

@sbtUnchecked sounds good.

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

So, after I have implemented the first two checks, I want to give a summary of what this PR is about.

I feel that sbt authors have a hard time writing sbt code because it's not predictable -- sometimes it does not behave like Scala code.

These checks are the first step to disallow dangerous code that misbehaves. The most common scenario is the use of a task invocation inside the body of an if expression. People may think the task in the body won't be executed if the predicate is false, but this is not the case -- sbt macros hoist up that value and evalute the task no matter what the if predicate is.

In order to address this issue, I think it's utterly important to give good feedback and explain sbt users in the spot why that construct is dangerous. This is also a good time to familiarize with the most complicated parts of sbt.

Feedback on the error message format is very much welcome.

@jvican jvican force-pushed the scalacenter:macro-usability-improvements branch from af562dc to 7b8e8ca May 25, 2017

private final val reset = if (ConsoleAppender.formatEnabled) AnsiColor.RESET else ""

def useOfValueInsideIfExpression(offendingValue: String) =
s"""${startBold}The evaluation of `${offendingValue}` happens always inside a regular task.$reset

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n May 25, 2017

Member

I think we should say "a task" instead "a regular task". I would think most people know it's a non-dynamic variety, and even if it's it's a dynamic task, if-expressions situation still stands.

This comment has been minimized.

Copy link
@jvican

jvican May 25, 2017

Author Member

The issue is: what happens if people think that only normal tasks exist? If this is the case and we say "a task", then they will be very confused because that's the only construct they are familiar with.

It seems that we disagree in this statement: "most people know it's a non-dynamic variety".

This comment has been minimized.

Copy link
@jvican

jvican May 25, 2017

Author Member

@eed3si9n Can we discuss this in a separate issue and we tackle all the error message wording before RC1? I'd like to focus on other stuff for now.

This comment has been minimized.

Copy link
@eed3si9n
Rename `sbt.unchecked` to `sbt.sbtUnchecked`
As per Eugene's suggestion.
overwrite = isSnapshot.value
),
publishConfiguration := {
// TODO(jvican): I think this is a bug.

This comment has been minimized.

Copy link
@jvican

jvican May 25, 2017

Author Member

I think this is a bug @eed3si9n. It's not clear to me that we should deliver unconditionally here. The previous if expression has the same semantics as this one, but I think the newest code uncovers the unexpected misbehaviour.

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n May 25, 2017

Member

Good catch. You're probably right.

@jastice

This comment has been minimized.

Copy link
Contributor

commented May 25, 2017

given all the ways not to get a conditional task execution right, the "use a dynamic task" warrants a link to a more detailed explanation.
(tell me where to put it and I'll make a PR for the doc)

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

given all the ways not to get a conditional task execution right, the "use a dynamic task" warrants a link to a more detailed explanation.
(tell me where to put it and I'll make a PR for the doc)

Totally agreed. There are several ways we could improve this. I would like to give every error an identifier, and then allow users to browse a full explanation of everything via help. Then we can cross-link between error messages.

@jastice As this is not high priority, I suggest that we synchronize on an issue on the best ways to do so before the RC1.

@eed3si9n eed3si9n merged commit 239280f into sbt:1.0.x May 25, 2017

1 check passed

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

@eed3si9n eed3si9n removed the in progress label May 25, 2017

@eed3si9n

This comment has been minimized.

Copy link
Member

commented May 25, 2017

Merged! Thanks for the contribution.

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

Thanks @eed3si9n.

@eed3si9n eed3si9n changed the title Add first round of DSL checks to sbt [sbt 1.0] Add first round of DSL checks to sbt May 25, 2017

@clhodapp

This comment has been minimized.

Copy link

commented May 25, 2017

Is the error here accurate? Are you not preventing the behavior that the error claims will happen with the error? Would it not be more accurate to say "regular tasks cannot conditionally depend on other tasks"?

@clhodapp

This comment has been minimized.

Copy link

commented May 25, 2017

Wonderful idea, by the way

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

Is the error here accurate? Are you not preventing the behavior that the error claims will happen with the error? Would it not be more accurate to say "regular tasks cannot conditionally depend on other tasks"?

Which error do you mean? 😄

@clhodapp

This comment has been minimized.

Copy link

commented May 26, 2017

Sorry! The one that's called out in the screenshot:
screenshot

It states that "Regular tasks always evaluate task inside the bodies of if expressions", which has historically been true. However: The fact that a check for this is being added means that this is no longer true, I would think. Regular tasks won't evaluate tasks inside the bodies of ifs because (due to this patch) putting a call to .value inside of an if is now disallowed, right?

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 26, 2017

Ah! @clhodapp I see what you mean.

Yes, you're right! The reason why we still tell users that is because users can bypass this check with @sbtUnchecked. So they can do something like:

if (condition) foo.value: @sbtUnchecked
else ""

Another reason why that error message is useful is because it helps sbt users realise what and why they were doing wrong. If someone migrates to 1.0 and all of a sudden gets that message, he'll get a good reason why the previous code was rejected.

@clhodapp I'm totally open to change the error message, what do you think would be a better wording? As a user, what would you like to see?

@eed3si9n eed3si9n referenced this pull request May 26, 2017

Closed

[sbt 1.0] Forward port 0.13 changes #2352

7 of 7 tasks complete
@clhodapp

This comment has been minimized.

Copy link

commented May 28, 2017

Hrm, I feel like it's kind of weird to allow it at all (even with @sbtUnchecked) but I understand now that the intent with this change is not to prevent things that we don't like from being possible but rather to make sure that users know and have opted into confusing behaviors. Given that, the only changes I would make would be to word things a bit more strongly and to tell the user how to make the error go away. More like "You should generally not do this because it is confusing. If you want to do it anyway, you'll need to add @sbtUnchecked".

Perhaps:

s"""${startBold}The evaluation of `$task` happens always inside a regular task.$reset
   |
   |${ProblemHeader}: `$task` is inside the if expression of a regular task.
   |  Regular tasks always evaluate calls to `.value` inside both of the branches of `if` expressions,
   | regardless of whether the predicate is true or false. Because this can cause confusion, calling
   | `.value` on tasks inside of `if` branches has been disabled by default.
   |
   |${SolutionHeader}:
   |  1. If you only want to evaluate it when the if predicate is true or false, use a dynamic task.
   |  2. If you are fine with `$task` being evaluated regardless of the value of the predicate, you can make this
   |     explicit by evaluating `$task` outside the `if` expression and assigning the result to a `val`.
   |     For example, you could write `val result = $task.value; if (condition) result else ""`.
   |  3. If you understand the evaluation model and would like sbt to simply allow you to call `.value` inside
   |     of the `if` branch, you can also annotate your call to `.value` with `@sbtUnchecked`. For example, you
   |     could write `if (condition) $task.value: @sbtUnchecked else ""`. Be aware that this may be confusing
   |     for others who try to read your build definition.

""".stripMargin
@clhodapp

This comment has been minimized.

Copy link

commented May 28, 2017

Note: I edited my last comment so if you are reading it in an email, you'll have to come over to GitHub to see the updated text. Sorry!

@jvican jvican deleted the scalacenter:macro-usability-improvements branch May 29, 2017

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 29, 2017

Hrm, I feel like it's kind of weird to allow it at all (even with @sbtUnchecked) but I understand now that the intent with this change is not to prevent things that we don't like from being possible but rather to make sure that users know and have opted into confusing behaviors.

There are two reasons what explain the existence of @sbtUnchecked:

  1. We, the developers of sbt, may have missed valid use case scenarios.
  2. Or there's a regression in the DSL linter and valid code is recognised as invalid. To avoid keeping users awaiting a new sbt release to fix their builds, the annotation helps them continue with their life.

That being said, I really like the problem statement that you've done. Thank you, I will update this message before RC1! 😄.

The only thing I'm not sure about is to put the use of sbtUnchecked in the solution header. To me, this should be kind of secret -- only available to people that really want to work around the DSL check. I would prefer to have only one or two solutions, instead of three, which may be confusing for users and forces them to read them all. @sbtUnchecked is an advanced use case so I feel comfortable leaving it to the docs. Does this make sense @clhodapp ?

@julienrf

This comment has been minimized.

Copy link

commented May 29, 2017

I’m a bit late but why not forbid this by construction instead of letting users write what they want and then checking that it makes sense?

@clhodapp

This comment has been minimized.

Copy link

commented May 30, 2017

@jvican hrm, I feel like having features like @sbtUnchecked exist but not be publicized is going to reinforce sbt's reputation for being confusing/having bad documentation. If we feel the need to hide part of our api from the user, it probably shouldn't exist. Further, without the solution text explaining @sbtUnchecked the problem text is confusing because it explains a theoretical scenario that feels impossible; It reads as "We don't let you do this but if we did here's what would happen."

eed3si9n added a commit to eed3si9n/sbt that referenced this pull request Apr 11, 2018

Fixes linter that detects missing .value
Fixes sbt#4079

sbt#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
  ^
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.