search

scalameta / metals

Scala language server with rich IDE features 🚀
https://scalameta.org/metals/
Apache License 2.0
2.07k stars 323 forks source link

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

Open ckipp01 opened 2 years ago

ckipp01 commented 2 years ago

Discussed in https://github.com/scalameta/nvim-metals/discussions/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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)

armanbilge commented 2 years ago

Related?

ckipp01 commented 2 years ago

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 commented 2 years ago

If you decide to go for i, it would/will benefit from https://github.com/scalacenter/scalafix/pull/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 commented 2 years ago

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 commented 2 years ago

Thanks @ckipp01, very clear - I added https://github.com/scalacenter/scalafix/issues/1625 to track the feature request.