organize-imports dosn't respect groups settings

[sideeffffect]

Actual behavior

With configuration

OrganizeImports {
  # IntelliJ IDEA's order so that they don't fight each other
  groups = [
    "java."
    "*"
    "scala."
  ]
}

organize-imports of version 0.3.0 turns

...
import scala.io.AnsiColor
import scala.language.postfixOps
import scala.sys.process._

into this

...
import scala.io.AnsiColor
import scala.sys.process._

import scala.language.postfixOps

almost as if the config were

  groups = [
    "java."
    "*"
    "scala."
    "scala.language"
  ]

But that's not the case.

Expected behavior

given the config above, organize-imports should preserve this ordering

...
import scala.io.AnsiColor
import scala.language.postfixOps
import scala.sys.process._

Note

This is a problem which prevents us from adopting organize-imports, because this fights with IntelliJ's Optimize imports.

[liancheng]

@sideeffffect, this is expected behavior because scala.language.postfixOps is an implicit lazy val. Please find the rationale here and in issue #30. Here is an example of how IntelliJ handles the implicit conflicts improperly:

package a

import a.c._
import a.b.i

object b {
  implicit val i: Int = ???
}

object c {
  implicit val i: Int = ???
  val d: Int = ???
}

object Implicits {
  def f()(implicit i: Int): Unit = println(d * i)
  f()
}

If you run IntelliJ import optimizer against the above example, those two imports will be reordered and a compilation error is introduced:

could not find implicit value for parameter i: Int
  f()
   ^

However, in your case, there's no conflicting implicits, and I admit that it's unintuitive to group scala.language.postfixOps separately. Unfortunately, I don't think Scalafix can surgically identify conflicting implicit hidden behind a wildcard import. To be conservative and ensure correctness, OrganizeImports always moves explicitly imported implicits to the trailing order-preserving group together with relative imports.

Since this conflicting implicits issue is pretty rare, maybe we can have an option for keeping IntelliJ compatibility and disable this behavior, and users may use it at their own risks.

[sideeffffect]

@liancheng IntelliJ compatibility mode would be great! 🙏
EDIT: something like forceGroupsLayout, that would make sure the groups is observed, no matter what

[liancheng]

@sideeffffect, PR #42 should be able to solve your issue.

You may use the 0.3.0+1-5c3e2618-SNAPSHOT snapshot release to try it out.

[sideeffffect]

Amazing work, thank you so much @liancheng ! I will test this first thing on Monady

[sideeffffect]

Ok, I can happily confirm it works fine, at least on my minimal project
We can start using this plugin once there is a release, hopefully nothing is blocking us at this point 🎉

[liancheng]

@sideeffffect, glad to hear that :)

Although the intellijCompatible option fixes this issue, I don't quite like it because:

1. This option encodes a product name in it.
2. This option may cause wrong expection: people may think this option enables OrganizeImports to produce exactly the same output as IntelliJ, while OrganizeImports possibly will never behave exactly the same as IntelliJ (being bug-to-bug compatible).

One alternative I'm considering is to special case implicits under the scala.languageFeature package. The motivations are:

1. scala.languageFeature values are recently changed from objects to implicit lazy vals in Scala 2.13. This means that OrganizeImports users who upgrades from 2.12 to 2.13 may observe different import organization results, which adds an unnecessary migration friction.
2. Importing scala.languageFeature values is practically the only common case I observed where people explicitly import an implicit name. Although I don't have the data, my hunch is that in 99% of the cases, people bring in implicits using a wildcard.
3. There's practically zero chance that you may have a conflicting language feature implicit defined somewher outside the scala.languageFeature package. (Each language feature value has their own trait type.)

So the proposed approach is to special-case scala.languageFeature values as if they are not implicits no matter which Scala version you are using. This brings in a little bit inconsistency, but removes an adoption and migration friction.

What do you think?

[sideeffffect]

I'm still worried that it would (or could, in some circumstances, if not for scala.languageFeature, then for a different thing) produce a different output from IntelliJ.
How about just renaming the option to forceGroupsLayout or strictGroups or observeGroups or something like that?
No trademarked name, just a description that what you prescribe in goups is what you get, even if it would cause compile errors.

It's amazing that organize-imports is doing the right thing by default (I wish IntelliJ did that from the beginning). Alas, compatibility with largely used legacy tools is also important.

The proposal regarding scala.languageFeature makes good sense 👍 , but IMHO, it's fundamentally orthogonal to forceGroupsLayout (or whatever we will call it).

Does that make sense?

[liancheng]

@sideeffffect, I cannot promise that OrganizeImports can always provide bug-to-bug compatibility to IntelliJ, but the only exceptions I see regarding to the groups option are relative imports and explicitly imported implicits.

OrganizeImports moves relative imports into a separate order-preserving group located after all the other import groups. This is necessary because relative imports are order sensitive. This behavior is different from IntelliJ but I don't consider this as a real issue, because IntelliJ always expand relative imports into fully-qualified ones. Once expanded, OrganizeImports treats them in basically the same way as IntelliJ, except for explicitly imported implicits. In fact, the only thing the new intellijCompatible option does is NOT to handle those implicits separately.

As for explicitly imported implicits, as described above, special-casing scala.languageFeature already works for your case, and I believe it should also be good enough for most developers unless they are intentionally torturing the Scala compiler...

Another reason why I prefer neither intellijCompatible nor renaming it to other names like strictGroups is that I want to minimize the number of tuning knobs. If there indeed turns out to be more special cases that OrganizeImports has to behave differently from IntelliJ, and new options are proven to be unavoidable, we can always add them later.

[sideeffffect]

fair enough
thank you @liancheng

[liancheng]

Thank you for your input, @sideeffffect! PR #48 implements the new proposal. I'll make 0.3.1-RC1 once it's merged.

[liancheng]

@sideeffffect, 0.3.1-RC1 has been cut. A few issues were found. Sorry for the delayed release!