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

Scalafix should allow running with rules = [] #1613

Open
armanbilge opened this issue Jun 6, 2022 · 7 comments
Open

Scalafix should allow running with rules = [] #1613

armanbilge opened this issue Jun 6, 2022 · 7 comments

Comments

@armanbilge
Copy link

class Test {
  def foo = for {
    x <- List(1, 2)
    val y = 2 * x
  } yield y
}

If I have .scalafix.conf:

rules = [NoValInForComprehension]

then scalafix runs fine.

However if I have .scalafix.conf:

rules = []

Then I get:

Missing --rules

However, in this case, rules was not missing, merely empty :)

@bjaglin
Copy link
Collaborator

bjaglin commented Jun 6, 2022

Thanks for the report!

However, in this case, rules was not missing, merely empty :)

Indeed, I guess a better wording could be "no rule requested to run" (since rules can come from CLI args or the configuration file).

I'd like to hear more about your use-case to understand why you think this should not be an error (possibly just a warning?). Are you running scalafix through sbt-scalafix or another build tool, targeting several modules with different .scalafix.conf maybe?

@armanbilge
Copy link
Author

armanbilge commented Jun 6, 2022

Thanks for the response!

I'd like to hear more about your use-case to understand why you think this should not be an error (possibly just a warning?). Are you running scalafix through sbt-scalafix or another build tool

Yes, we're rolling out sbt-scalafix en-masse to an entire organization.
https://github.com/http4s/sbt-http4s-org/releases/tag/v0.14.0

There's a lot of automation in there, including auto-generated GitHub actions workflows which enforce scalafix checks in CI.

Ideally, there would be a convenient way for individual maintainers to disable any scalafix rules for their repo.

For example, we also roll out scalafmt in the same way, but it's easy to opt out with this config:

project.includePaths = [] # disables formatting

targeting several modules with different .scalafix.conf maybe?

Sure, this would be convenient for that too, if you don't want any rules for Test config for example

@bjaglin
Copy link
Collaborator

bjaglin commented Jun 12, 2022

For the context: for sbt-scalafix users, running scalafix without any configuration-provided nor CLI-provided rule means that the invocation is a no-op, which is most likely a setup issue, thus the verbose failure. I am hesitant to change that behavior, and I am trying to challenge the introduction of another flag that could change that behavior.

https://github.com/http4s/sbt-http4s-org/releases/tag/v0.14.0

Just a side note about the release notes: they mention triggered.rules, but is scalafixOnCompile enabled somehwere?

Ideally, there would be a convenient way for individual maintainers to disable any scalafix rules for their repo.

I am assuming that "maintainers" here refer to "maintainers using sbt-typelevel", right? It looks like tlCiScalafix does avoid CI invocations at least - in which other context would you like to "disable scalafix"?

For example, we also roll out scalafmt in the same way, but it's easy to opt out with this config:

From what I can see, TypelevelScalafixPlugin triggers automatically and adds semanticdb, so just adding the sbt-typelevel plugin has a side effect on compilation, no matter if we have a "no-op" option in scalafix like the one you describe for scalafmt.

I guess it goes against the design of sbt-typelevel, but maybe this plugin should be added explicitly by maintainers? In that case, you could define typelevelScalafix/typelevelScalafixCheck task keys (wrapping the scalafix input task key), which would thus only aggregate on projects where the project is explicitly setup (and where we expect a non-empty set of rules defined).

Sure, this would be convenient for that too, if you don't want any rules for Test config for example

Actually, that's already possible, as the scalafixConfig key is looked up at the configuration level. I'll add a scripted to guard that useful behavior (scalacenter/sbt-scalafix#305).

I was asking because that's a use-case I can see with sbt-scalafix: calling scalafixAll should probably not fail if we have rules set up for IntegrationTest and Compile but not for Test (based on different configuration files). I need to think about that.

@armanbilge
Copy link
Author

Thank you so much for the detailed response!


For the context: for sbt-scalafix users, running scalafix without any configuration-provided nor CLI-provided rule means that the invocation is a no-op, which is most likely a setup issue, thus the verbose failure.

For the record, I 100% agree with you. This should definitely be a verbose failure.

By opening this issue I was hoping you might distinguish between:

# empty file, no config

and:

rules = [] # I *deliberately* set rules to empty

In the latter case, configuration has been provided.


Actually, that's already possible, as the scalafixConfig key is looked up at the configuration level. I'll add a scripted to guard that useful behavior (scalacenter/sbt-scalafix#305).

Yes, thanks for the PR! I am indeed aware it is possible to have separate scalafix configs for Compile, Test, etc. But my question is, what if I don't want any rules enabled for Test, only for Compile. How can I configure that?


Here's an example where a project wanted different scalafix configurations for different projects. For some legacy modules, they wanted to enable no rules, to avoid code churn.

In the end, it was necessary to still specify one rule as a placeholder.

@bjaglin
Copy link
Collaborator

bjaglin commented Jun 15, 2022

Technically speaking, we could try wrapping

in an Option to see whether we can have a no-op, non-failing behavior with an explicit empty list of rules.

I do feel like this would be fixing the problem at the wrong level though since we don't want to be calling scalafix to figure out it has nothing to do but rather not call scalafix at all. What about honoring scalafix / skip at the configuration level? You could also look at this flag to conditionally enable semanticdb in TypelevelScalafixPlugin, so that a skipped project/configuration really doesn't add overhead.

Somewhat related: #1413.

@armanbilge
Copy link
Author

armanbilge commented Jun 15, 2022

What about honoring scalafix / skip at the configuration level? You could also look at this flag to conditionally enable semanticdb in TypelevelScalafixPlugin, so that a skipped project/configuration really doesn't add overhead.

So here's an interesting counterpoint: I asked for sbt-mima to support skip in lightbend-labs/mima#684, and instead was told to use mimaPreviousArtifacts := Set.empty 😂

Of course, that shouldn't dictate what is the right thing to do here. My larger point is that unfortunately skip support across the sbt ecosystem is very unreliable, and as much as I like the concept I've been burned in the past e.g. typelevel/sbt-typelevel#140. Reading #1413 it seems you may have encountered similar issues.


I do feel like this would be fixing the problem at the wrong level though since we don't want to be calling scalafix to figure out it has nothing to do but rather not call scalafix at all.

I'm not sure if it's the wrong level. It depends on whether you consider this an issue specific to sbt-scalafix or scalafix in general, in which case the .scalafix.conf seems the right place to address this.

Although less common there are ways to run scalafix outside of sbt. And maybe in the age of scala-cli-based projects these non-sbt ways of doing things will become more important. Also metals does things outside of sbt.

For example, in orgs it's often useful to distribute a common configuration* e.g. .scalafix-common.conf that CI enforces stays up-to-date against a master copy. Then, individual projects may override settings on top of it:

# .scalafix.conf
include ".scalafix-common.conf"
rules += "AnotherRule"

This is another situation where for some reason, perhaps a project wants/needs to explicitly disable all rules.

* FTR is the setup recommended by scalafmt for sharing configuration:
https://scalameta.org/scalafmt/docs/installation.html#share-configuration-between-builds

@bjaglin
Copy link
Collaborator

bjaglin commented Jun 27, 2022

I'm not sure if it's the wrong level. It depends on whether you consider this an issue specific to sbt-scalafix or scalafix in general, in which case the .scalafix.conf seems the right place to address this.

Although less common there are ways to run scalafix outside of sbt. And maybe in the age of scala-cli-based projects these non-sbt ways of doing things will become more important. Also metals does things outside of sbt.

My point was that any semantic-rule-ready client must enable semanticdb upfront before calling scalafix, since scalafix expects semanticdb files to be available. So I was advocating to shift left in order to prevent an unnecessary increase of build time when scalafix is going to end up doing nothing. I am just still unsure how. We could expose whether scalafix is going to run or not as part of the API, but it might be too late for some clients, like sbt-scalafix.

For the record, the code handling rules = [] is there:

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

No branches or pull requests

2 participants