Should I get an error when running run_scalafix without having any rules defined?

[ckipp01]

Discussed in scalameta/nvim-metals#420

^{Originally posted by caenrique June 27, 2022}
I have an autocommand on BufWritePre to run scalafix with the organize imports rule, but this is only set up in some of my projects. When it gets executed in a project without any rules defined I get this error in the nvim messages:

[nvim-metals] Could not execute command: Internal error.

metals log:

o2022.06.27 12:40:27 ERROR missing rulesscala.meta.internal.metals.ScalafixProvider$ScalafixRunException: missing rules
        at scala.meta.internal.metals.ScalafixProvider.$anonfun$runScalafixRules$1(ScalafixProvider.scala:149)
        at scala.meta.internal.metals.ScalafixProvider$$Lambda$4835/000000002057D020.apply(null)
        at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:470)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:823)

Jun 27, 2022 12:40:27 PM org.eclipse.lsp4j.jsonrpc.RemoteEndpoint fallbackResponseError
SEVERE: Internal error: scala.meta.internal.metals.ScalafixProvider$ScalafixRunException: missing rules
java.util.concurrent.CompletionException: scala.meta.internal.metals.ScalafixProvider$ScalafixRunException: missing rules
        at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:292)
        at java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:308)
        at java.util.concurrent.CompletableFuture.uniAccept(CompletableFuture.java:661)
        at java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java:646)
        at java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:488)
        at java.util.concurrent.CompletableFuture.completeExceptionally(CompletableFuture.java:1990)
        at scala.concurrent.java8.FuturesConvertersImpl$CF.apply(FutureConvertersImpl.scala:29)
        at scala.concurrent.java8.FuturesConvertersImpl$CF.apply(FutureConvertersImpl.scala:26)
        at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:484)
        at scala.concurrent.ExecutionContext$parasitic$.execute(ExecutionContext.scala:222)
        at scala.concurrent.impl.Promise$Transformation.submitWithValue(Promise.scala:429)
        at scala.concurrent.impl.Promise$DefaultPromise.submitWithValue(Promise.scala:338)
        at scala.concurrent.impl.Promise$DefaultPromise.tryComplete0(Promise.scala:285)
        at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:504)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:823)

I'm not sure what the expectations for this API is. Should I handle this case myself, or is this behaviour unexpected.
I would expect that it just does nothing if there aren't any scalafix rules defined in the project

[ckipp01]

Thanks for reporting this @caenrique! You're correct, this shouldn't happen. I'd expect the user to see a nicer message this this, not an Internal error message. We need to handle this a bit differently in Metals, so I've moved this issue over to the main repo since that's where the fix will be.

[tgodzik]

Thanks for reporting! So the question here is whether we should just show a nicer error or maybe we should add OrganizeImports by default ?

[tanishiking]

So the question here is whether we should just show a nicer error or maybe we should add OrganizeImports by default ?

I came up with three options (1 is the easiest, 3 is the richest, I guess)

• 1. Just do nothing, and ignore run_scalafix if rules are unavailable.
  • I think it's natural to ignore run scalafix if rules are not available. However, it might make it hard to troubleshoot for users.
• 2. Show a nicer, actionable error (maybe warn) instead of showing a stack trace. Something like rules are missing, please create .scalafix.conf and configure rules with rules = [ ... ]` (?)
• 3. Show popup or something like FormattingProvider creates .scalafmt.conf, but I'm not sure what kind of rules should be on in the created file.

[armanbilge]

Related?

• Scalafix should allow running with rules = [] scalacenter/scalafix#1613

[ckipp01]

Yea I'm sort of liking iii since it would most closely align with the behavior of triggering scalafmt for the first time. The only tricky this is that if we do this, then users will expect it to work with organize imports and it will with Metals maybe, but not with their build tool or others on their team using IntelliJ for example. Full circle makes me wish dependencies could be defined in your .scalafix.conf file.

Another easy alternative to i is just show a nice warning to the user that they have no .scalafix.conf file and that they should just set it up if they'd like to use the command.

[bjaglin]

If you decide to go for i, it would/will benefit from scalacenter/scalafix#1624

Full circle makes me wish dependencies could be defined in your .scalafix.conf file.

That's definitely something that would be beneficial to all scalafix clients, and it wouldn't be too hard to implement by inferring a high-level tool-classpath from dependencies optionally listed in the conf file. I understand it would reduce the complexity of the ScalafixProvider here, but how would it help in that particular issue?

[ckipp01]

I understand it would reduce the complexity of the ScalafixProvider here, but how would it help in that particular issue?

Sure, to expand on the example I had, let's pretend that we create a .scalafix.conf file for the user, but it's empty. However when the user triggers run scalafix and we organize imports for example the user will still get their imports organized. Then they check in their code with this new .scalafix.conf with their build tool still needing to add that external rule. Whereas if you could include an external dep in your .scalafix.conf file then when metals creates it with the necessary stuff, the user wouldn't need to remember to change their build definition at all, just add sbt-scalafix or use scalafix cli in CI etc. Does that make sense?

[bjaglin]

Thanks @ckipp01, very clear - I added scalacenter/scalafix#1625 to track the feature request.