[Scala 3] Add support for given imports #187

tgodzik · 2021-06-11T11:10:48Z

Previously, organize-imports-rule would throw an exception in case of given imports, now it should sort them correctly. Currently they will be sorted separately at the end, but we can add some more options. However I am not sure what will be the best practises here. We could possibly have an option to put them at the end, but I think the current situation is fine for now.

~~You cannot unimport given imports, so we don't have do do anything similar to wildcard improts when it comes to given wildcard imports.~~ You can actually

~~Tests are currently blocked on #179 , I only checked it manually.~~ We have tests now! 🎉

@bjaglin should we wait for that?

I thought of raising an issue, but turned out faster to fix it 😅 If there are some other plans here, feel free to close the PR

liancheng · 2021-06-11T17:31:49Z

@tgodzik, thanks for this! Could you please also add some test cases?

codecov · 2021-06-11T17:34:07Z

Codecov Report

Merging #187 (a4dea13) into master (f9a939b) will increase coverage by 0.01%.
The diff coverage is 93.61%.

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   91.04%   91.05%   +0.01%     
==========================================
  Files           4        4              
  Lines         268      302      +34     
  Branches       10       15       +5     
==========================================
+ Hits          244      275      +31     
- Misses         24       27       +3

Impacted Files	Coverage Δ
rules/src/main/scala/fix/OrganizeImports.scala	`95.07% <93.61%> (-0.58%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9a939b...a4dea13. Read the comment docs.

bjaglin · 2021-06-11T17:45:40Z

@tgodzik, thanks for this! Could you please also add some test cases?

It's blocked by #179. I didn't have time to update the README to mention the fact that removeUnused is not supported on Scala3, but I think this could be done separately. So I would suggest we merge #179 to unlock scala3-specific tests and iterate in other PRs.

liancheng · 2021-06-12T08:04:58Z

rules/src/main/scala/fix/OrganizeImports.scala

@@ -774,40 +777,55 @@ object OrganizeImports {
        List[Importee.Name],
        List[Importee.Rename],
        List[Importee.Unimport],
+        List[Importee.Given],
+        Option[Importee.GivenAll],


Would you please also update the comment on this method to reflect this change?

liancheng · 2021-06-12T08:13:14Z

rules/src/main/scala/fix/OrganizeImports.scala

    config.coalesceToWildcardImportThreshold
-      .filter(names.length > _)
+      .filter(names.length + givens.length > _)
      .map(_ => importer.copy(importees = renames ++ unimports :+ Importee.Wildcard()))


Should the optional Importee.GivenAll instance also exist in the result importer?

I handle it now separately with 4 different cases.

liancheng · 2021-06-14T07:46:10Z

I'm not familiar with Scala 3 given imports (just learned it by reading the spec while reviewing this PR), but I wonder do we also need to update the mergeImporters method? It is the most complicated method of this rule and handles a bunch of corner cases.

For example, what should be the result import if we merged the following two?

import foo.{aGiven as _, given}
import foo.given

TBH, I'm not even sure whether the above two imports are valid, but my intention is:

foo.aGiven is a given instance
The first import intends to import all given instances under foo except for aGiven (assuming that we can unimport a given instance)
The second import intends to import all given instances under foo

I think the result should be import foo.given.

This example is derived from the following case in Scala 2:

import foo.{x => _, _}
import foo._

The above Scala 2 imports should be merged into a single import foo._, because the unimport is canceled by the second wildcard import.

We may want to review all the cases handled in mergeImporters to handle all the potentially introduced new cases.

tgodzik · 2021-06-14T12:25:12Z

So from what I understand there is no way to unimport given instances, however I am not 100% sure. I will check out the situation when explicitly importing a given instead of by type

tgodzik · 2021-06-21T12:02:37Z

I just checked and it seems that we can indeed unimport givens, I will fix it and add some tests as soon as #179 is merged. Or should we merge it beforehand in order to fix the Scala 3 support?

tgodzik · 2021-07-14T16:00:12Z

@liancheng I added a bunch of tests and added support for given inside mergeImporters method. It should now handle wildcard given import them the same as a simple wildcard one. There is a single difference, if we merge imports we will not merge given and normal imports, since we don't know if given imports aren't using the types from the same package. And those given imports will break if the type they are using is imported in the same place.

I think it works correctly now, but my brain is having some issues figuring all the possible edge cases 😅

I haven't added any support for unused imports yet since there is no option for it in the Scala 3 compiler yet, so I am not sure even if unused given imports will be reported.

Let me know what you think!

tgodzik · 2021-07-14T16:06:20Z

output/src/main/scala-3/fix/GroupedGivenImportsMergeUnimports.scala

+
+import fix.GivenImports._
+import fix.GivenImports.{gamma => _, given Beta, given Zeta, given}
+import fix.GivenImports2.{alpha => _, beta => _}


As you can see the imports are separated into givens and normal ones aside from the situation, where the unimports were together with given imports. Not sure if we can do better. Maybe we could check if the the types of the givens are not imported in the previous imports and join them in that case?

bjaglin

Cool! The actual implementation goes beyond my knowledge, but just as a note: you might want to remove/update https://github.com/liancheng/scalafix-organize-imports/blame/master/README.adoc#L92 since test coverage seems quite decent.

liancheng · 2021-07-14T23:46:55Z

@tgodzik, thanks for the update! The mergeImporters is indeed tricky. If I had learned that organizing Scala imports is this complicated, I possibly wouldn't even start this project 😅

For unused imports for Scala 3, it's OK to leave it out for now as long as:

We document this clearly in the README.
No regression for existing Scala 2 support (I believe the current test coverage is good enough for this).

I will find some time to do a more thorough review, hopefully later this week.

liancheng · 2021-07-15T14:07:57Z

@tgodzik, we also need to update the ImporterExtension.isCurlyBraced method to include given imports. This may affect importer sorting order.

tgodzik · 2021-07-15T14:20:11Z

@tgodzik, we also need to update the ImporterExtension.isCurlyBraced method to include given imports. This may affect importer sorting order.

Sure, I will take a look next week!

Previously, organize-imports-rule would throw an exception in case of given imports, now it should sort them correctly. Most of the existing rules should apply correctly to imports with givens, with an exception that grouped imports will be separated, so that in case of importing a type needed for given we will not get an error.

tgodzik · 2021-07-19T17:25:12Z

isCurlyBraced

It seems that we can actually skip {} in given imports, so the behaviour is the same as Importee.Name, but I need to fix it in Scalameta and we can improve it further on. (scalameta/scalameta#2425)

I also added a bit in the readme.

liancheng · 2021-07-26T22:00:32Z

I'm merging this. There are a few minor places that can be cleaned up a little bit, but I can do it in a follow-up PR. Thanks again, @tgodzik!

tgodzik · 2021-07-27T06:15:56Z

Thanks a lot!

liancheng reviewed Jun 12, 2021

View reviewed changes

bjaglin mentioned this pull request Jun 24, 2021

Test Scala 3 support using sbt-projectmatrix #179

Merged

tgodzik force-pushed the add-given-support branch 4 times, most recently from 2a06d9b to 3ee2587 Compare July 14, 2021 15:56

tgodzik requested a review from liancheng July 14, 2021 16:01

tgodzik force-pushed the add-given-support branch from 3ee2587 to b370d99 Compare July 14, 2021 16:03

tgodzik commented Jul 14, 2021

View reviewed changes

bjaglin reviewed Jul 14, 2021

View reviewed changes

tgodzik force-pushed the add-given-support branch from b370d99 to a4dea13 Compare July 19, 2021 17:23

liancheng approved these changes Jul 26, 2021

View reviewed changes

liancheng merged commit a357eca into liancheng:master Jul 26, 2021

liancheng mentioned this pull request Aug 3, 2021

Coalesce given imports #198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Scala 3] Add support for given imports #187

[Scala 3] Add support for given imports #187

tgodzik commented Jun 11, 2021 •

edited

Loading

liancheng commented Jun 11, 2021

codecov bot commented Jun 11, 2021 •

edited

Loading

bjaglin commented Jun 11, 2021 •

edited

Loading

liancheng Jun 12, 2021

tgodzik Jul 14, 2021

liancheng Jun 12, 2021

tgodzik Jul 14, 2021

liancheng commented Jun 14, 2021

tgodzik commented Jun 14, 2021

tgodzik commented Jun 21, 2021

tgodzik commented Jul 14, 2021

tgodzik Jul 14, 2021

bjaglin left a comment

liancheng commented Jul 14, 2021

liancheng commented Jul 15, 2021

tgodzik commented Jul 15, 2021

tgodzik commented Jul 19, 2021 •

edited

Loading

liancheng commented Jul 26, 2021

tgodzik commented Jul 27, 2021

[Scala 3] Add support for given imports #187

[Scala 3] Add support for given imports #187

Conversation

tgodzik commented Jun 11, 2021 • edited Loading

liancheng commented Jun 11, 2021

codecov bot commented Jun 11, 2021 • edited Loading

Codecov Report

bjaglin commented Jun 11, 2021 • edited Loading

liancheng Jun 12, 2021

Choose a reason for hiding this comment

tgodzik Jul 14, 2021

Choose a reason for hiding this comment

liancheng Jun 12, 2021

Choose a reason for hiding this comment

tgodzik Jul 14, 2021

Choose a reason for hiding this comment

liancheng commented Jun 14, 2021

tgodzik commented Jun 14, 2021

tgodzik commented Jun 21, 2021

tgodzik commented Jul 14, 2021

tgodzik Jul 14, 2021

Choose a reason for hiding this comment

bjaglin left a comment

Choose a reason for hiding this comment

liancheng commented Jul 14, 2021

liancheng commented Jul 15, 2021

tgodzik commented Jul 15, 2021

tgodzik commented Jul 19, 2021 • edited Loading

liancheng commented Jul 26, 2021

tgodzik commented Jul 27, 2021

tgodzik commented Jun 11, 2021 •

edited

Loading

codecov bot commented Jun 11, 2021 •

edited

Loading

bjaglin commented Jun 11, 2021 •

edited

Loading

tgodzik commented Jul 19, 2021 •

edited

Loading