Skip to content
This repository was archived by the owner on Sep 3, 2020. It is now read-only.

Organize Imports in Blocks (DefDef) and Templates avoiding tree printer #144

Merged
merged 27 commits into from
Apr 8, 2016
Merged

Organize Imports in Blocks (DefDef) and Templates avoiding tree printer #144

merged 27 commits into from
Apr 8, 2016

Conversation

wpopielarski
Copy link
Member

Work bases on #142. Same concept and code. Done.

@wpopielarski wpopielarski changed the title [WIP] Organize Imports in Templates avoiding tree printer Organize Imports in Templates avoiding tree printer Mar 11, 2016
@wpopielarski wpopielarski changed the title Organize Imports in Templates avoiding tree printer Organize Imports in Blocks (DefDef) and Templates avoiding tree printer Mar 12, 2016
@wpopielarski
Copy link
Member Author

add Martin Delemotte's test

object AnObject {
  val a = 3
}

object Test {
  def test() : Int = {
    import AnObject._
    a
  }
}

@mlangc
Copy link
Collaborator

mlangc commented Mar 18, 2016

@wpopielarski did you consider comments (see https://scala-ide-portfolio.assembla.com/spaces/scala-ide/tickets/1001848-automatic-import-deletes-comments-between-import-statements/details#)? I think we have a regression here, as organizing imports on

/*
 * Please let me live!
 */
import java.util.Date

class Bug {

}

now removes the comment at the start of the file.

You might want to take a look at SourceWithMarker and the related movements (for example SourceWithMarker.Movements.commentsAndSpaces) if you need some more advanced parsing. As soon as PR #141 is merged, these parsers even do backtracking...

@wpopielarski
Copy link
Member Author

Generally I don't touch in this PR imports in package scope. So I don't see
why it could be somehow violated. The Region encompasses the whole section
so I don't think it removes comments. I would expect rather opposite
behavior. It preserves comment event the import below had been removed. Let
me write test to confirm

2016-03-18 13:56 GMT+01:00 Matthias Langer [email protected]:

@wpopielarski https://github.com/wpopielarski did you consider comments
(see
https://scala-ide-portfolio.assembla.com/spaces/scala-ide/tickets/1001848-automatic-import-deletes-comments-between-import-statements/details#)?
I think we have a regression here, as organizing imports on

/* * Please let me live! */import java.util.Date
class Bug {

}

now removes the comment at the start of the file.

You might want to take a look at SourceWithMarker and the related
movements (for example SourceWithMarker.Movements.commentsAndSpaces) if
you need some more advanced parsing. As soon as PR #141
#141 is merged,
these parsers even do backtracking...


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#144 (comment)

@mlangc
Copy link
Collaborator

mlangc commented Mar 18, 2016

Hmm, I thought one of the big changes of this PR is that Organize Imports no longer uses tree printing... this would also affect package level imports, or am I wrong?

@mlangc
Copy link
Collaborator

mlangc commented Mar 18, 2016

This test

@Test
  def shouldNotRemoveComments() = new FileSet {
    """
    /*<-*/
    package test

    trait Bug {
      import java.util.Date
      import java.util.ArrayList
      // HELP
      import scala.util.Try
      import scala.concurrent.Future

      val d: Date
      val l: ArrayList[Int]
      val t: Try[Int]
      val f: Future[Double]
    }
    """ becomes {
    """
    /*<-*/
    package test

    trait Bug {
      import java.util.ArrayList
      import java.util.Date
      import scala.concurrent.Future
      // HELP
      import scala.util.Try

      val d: Date
      val l: ArrayList[Int]
      val t: Try[Int]
      val f: Future[Double]
    }
    """
    }
  } applyRefactoring organizeWithTypicalParams

fails for me, as the comment is removed.

@wpopielarski
Copy link
Member Author

You're correct. I talked about it today morning with Simon and Iulian and
we decided to include package part to this refactoring too (besides class
and def, but final decision is done today). So up to next week I try to add
package part. By now I'd like to ask you about comments to things done for
classes and methods. Do you think it is ok? I mean design, some assumptions
and not covered corner cases.

2016-03-18 14:35 GMT+01:00 Matthias Langer [email protected]:

Hmm, I thought one of the big changes of this PR is that Organize Imports
no longer uses tree printing... this would also affect package level
imports, or am I wrong?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#144 (comment)

@wpopielarski
Copy link
Member Author

I see
but this one passes (what surprises me a little)

  def shouldRemoveDuplicatedImportFromDefWithComment() = new FileSet {
    """
    /*<-*/
    package test

    object AnObject {
      val a = 3
      val b = 4
    }

    object Test {
      import AnObject.b
      def test(): Int = {
        import AnObject.a
        // comment for b
        import AnObject.b
        a + b
      }
    }
    """ becomes {
    """
    /*<-*/
    package test

    object AnObject {
      val a = 3
      val b = 4
    }

    object Test {
      import AnObject.b
      def test(): Int = {
        import AnObject.a
        a + b
      }
    }
    """
    }
  } applyRefactoring organizeWithTypicalParams

@wpopielarski
Copy link
Member Author

oh man RangePosition is funny, this guy fails:

  @Test
  def shouldRemoveDuplicatedImportFromDefWithComment_v2() = new FileSet {
    """
    /*<-*/
    package test

    object AnObject {
      val a = 3
      val b = 4
    }

    object Test {
      import AnObject.b
      def test(): Int = {
        // comment for a
        import AnObject.a
        // comment for b
        import AnObject.b
        b
      }
    }
    """ becomes {
    """
    /*<-*/
    package test

    object AnObject {
      val a = 3
      val b = 4
    }

    object Test {
      import AnObject.b
      def test(): Int = {
        b
      }
    }
    """
    }
  } applyRefactoring organizeWithTypicalParams

it wants to be

 becomes {
    """
    /*<-*/
    package test

    object AnObject {
      val a = 3
      val b = 4
    }

    object Test {
      import AnObject.b
      def test(): Int = {
        //comment for a
        b
      }
    }
    """
    }
  } applyRefactoring organizeWithTypicalParams```

add it can make sense because first comment can refer to whole block of imports

@mlangc
Copy link
Collaborator

mlangc commented Mar 18, 2016

Well, the fact that the comment is removed is not a regression... it also happens without this PR; I just filed a bug: https://scala-ide-portfolio.assembla.com/spaces/scala-ide/tickets/1002671

As for range positions: Unfortunately they are not always right :-/

@mlangc
Copy link
Collaborator

mlangc commented Mar 18, 2016

As for reviewing the code itself: Can you give me a brief overview of the involved classes/traits and how they interact? This is a rather big PR....

@wpopielarski
Copy link
Member Author

@mlangc sure
so currently we don't support class/def imports (I use 'imports' but mean 'organize imports feature'). This PR tries to cover this gap.
Assumptions:

  • do not interact with existing package scope imports
  • do not modify tree
  • do not use tree printer

Design decision:

  • do not try to build almighty solution for all refactoring problems,
  • so as a consequence of above create solution for imports only.

Observation:

  • imports are grouped
  • moving imports up can break code (e.g. stable identifiers shadowing) so moving import anywhere outside its group is forbidden
  • as a consequence of above there are following operations on imports in group available:
    a. sorting (move in group only)
    b. duplicates removal (in group)
  • there is also duplicates removal operation defined concerning import groups (see shouldRemoveDuplicatedImportsInScopes test

Implementation:

  • import group is organize in Region. Region consists of a list of imports (which can be modified as a result of group processing). Region knows too how to build TextChange
  • every kind of tree which can contain imports has its own processing (see ImportsOrganizers) so improvements can be delivered independently
  • Region knows also how to print particular import (see printImport in Region)

I hope it gives glib entry to PR

@mlangc
Copy link
Collaborator

mlangc commented Mar 19, 2016

I just tried "Organize Imports" on oimports.Region while reviewing the code: It removes https://github.com/scala-ide/scala-refactoring/pull/144/files#diff-1a3cab437bffa245413182c652e2ba10R64, thereby causing a compile error.

def printImport(imp: Global#Import): String = {
import global._
val RenameArrow = " => "
val prefix = source.content.slice(imp.pos.start, imp.pos.end).mkString.reverse.dropWhile { _ != '.' }.reverse
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks with

  import scala.util.{Try => `Böse....`}
  import scala.concurrent./*Belze.Bub*/Future

Consider using SourceWithMarker here, like (just a sketch to give you the idea)

val srcAtImportEnd = SourceWithMarker.atEndOf(imp.pos.asInstanceOf[RangePosition])
val srcAtLastPoint = srcAtImportEnd.moveMarkerBack(until('.', skipping = (comment | literalIdentifier)))
assert(srcAtLastPoint.currentOption == Some('.'))
val prefix = source.content.slice(imp.pos.start, srcAtLastPoint.marker + 1).mkString("")

Note that I've tested this with the latest SourceWithMarker version from PR #141 that includes a significant improvement (backtracking), but I think this should also work with the old version.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to take a look at 99def11 (part of #141 ) before experimenting with Movements.until. It might safe you some time...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this is hell problem, but do we really need to support this devilishly evil comment example (/*Belze.Bub*/)? Just a question :). Is it common practice to write code in this manner or we just want to as strict as possible? Anyway I try Movements.util anyway

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, /*Belze.Bub*/ is indeed a very evil comment ;-)... but what about

import scala.collection./*immutable.*/Seq

I'm sure someone already did it.... Also, take a look at http://doc.akka.io/api/akka/2.4.2/?#akka.http.scaladsl.model.MediaTypes$. Thus I think doing a string search for the last "." is not an option here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks a lot for your delving the problem, I didn't realize that it is so common. So generally looks that I need to wrap imports in something conveying print information.

@wpopielarski
Copy link
Member Author

@mlangc I get than that what we need is something like ImportRegion with text sliced from origin file. Working on it...

assert(imports.nonEmpty)
val source = imports.head.pos.source
def printImport(imp: Global#Import): String = {
import global._
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

problem with this import global._ lays in the way how tree traverser discovers that given type comes from given import. Looks like it is not perfect. This piece of code was not touched by me. But it is bug to fix so I try to face it.

import scala.tools.refactoring.common.TextChange
import scala.util.Properties

case class Region private (imports: List[Global#Import], owner: Global#Symbol, startPos: Global#Position, endPos: Global#Position,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this class can be private[oimports]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, I would allow to leak it. Maybe other packs could use it? If you insist I make it pack private.

(imp, usedSelectors)
}.collect {
case (imp, selectors @ h :: _) => imp.copy(selectors = selectors).setPos(imp.pos)
}.toList
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@mlangc
Copy link
Collaborator

mlangc commented Mar 31, 2016

@wpopielarski All right, adding a switch for the new behaviour is fine for me too. I'd however suggest to be conservative at the beginning and not enable it by default.

wpopielarski added a commit to wpopielarski/scala-ide that referenced this pull request Apr 4, 2016
Adds settings to `OrganizeImportsPreferencePage` to control featue
implemented by PR scala-ide/scala-refactoring#144
@mlangc
Copy link
Collaborator

mlangc commented Apr 7, 2016

There are some tests (like shouldRemoveImportWithUnusedImplicit), that actually verify broken behaviour. I think we should test for correct behaviour and set them to @Ignore instead.

(t, onlyDifferentChildren) match {
case (t: Block , (orig, changed) :: Nil) if changed.pos == NoPosition =>
replaceSingleDef(orig, changed) ++ drillDownChildrenAndCollectChangesWithPosition(t)
replaceSingleDef(orig, changed)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mlangc
Copy link
Collaborator

mlangc commented Apr 7, 2016

Unfortunately organizing imports on scala.tools.refactoring.implementations.OrganizeImports breaks the code :-/ ... this is not a regression though, so it shouldn't hold this PR back.

@@ -407,7 +407,8 @@ abstract class OrganizeImports extends MultiStageRefactoring with TreeFactory
class RefactoringParameters(
val importsToAdd: List[(String, String)] = Nil,
val options: List[Participant] = DefaultOptions,
val deps: Dependencies.Value = Dependencies.RemoveUnneeded)
val deps: Dependencies.Value = Dependencies.RemoveUnneeded,
val organizeLocalImports: Boolean = true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wpopielarski if you enable this by default (against my suggestion from above - but I don't insist), please make sure that you don't forget about making this configurable in the IDE. This will also help with debugging, as we can then see very quickly if bugs go away as we enable/disable this.

@fommil This is something ENSIME might care about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is PR in scala-ide to toggle feature: scala-ide/scala-ide#1089

@fommil
Copy link
Contributor

fommil commented Apr 7, 2016

So long as local imports are not being moved away from their location, I think it sounds good


private def cutPrefixSuffix(imp: Global#Import, source: SourceFile): (String, List[String]) = {
val printedImport = source.content.slice(imp.pos.start, imp.pos.end).mkString
val prefixPatternWithCommentInside = """import (((\/\*.*\*\/)*(\w|\d|_|-)+(\/\*.*\*\/)*)\.)+(\/\*.*\*\/)*""".r
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this supposed to match?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import \*comment.comment*\org.acme\*comment*\.
import org.acme.\*comment.comment*\acne.\*comment.comment*\

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't match the first test case:

scala> val x = """import (((\/\*.*\*\/)*(\w|\d|_|-)+(\/\*.*\*\/)*)\.)+(\/\*.*\*\/)*""".r
x: scala.util.matching.Regex = import (((\/\*.*\*\/)*(\w|\d|_|-)+(\/\*.*\*\/)*)\.)+(\/\*.*\*\/)*

scala> x.findFirstIn("""import \*comment.comment*\org.acme\*comment*\.""")
res3: Option[String] = None

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry @sschaef it covers cases in shouldNotRemoveCommentsInImportPackagePrefix so `import comment.comment\ is not included

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally Im not sure about it. Strange but nothing works correctly in REPL. i'm pretty sure your example will work too in test, i verify it on Monday

@kiritsuku
Copy link
Member

LGTM. I didn't do a in depth code analysis though. Just a check that the functionality works as expected (in the cases where I tested it).

@kiritsuku
Copy link
Member

@wpopielarski @mlangc I lost track of all the comments here. Any blocking issues left? Or just additional features and bug fixes that could also go in a future PR?

@mlangc
Copy link
Collaborator

mlangc commented Apr 8, 2016

The only blocking issue left for me is the tests that check for broken behaviour (see my comment above). Apart from that I think that we should merge this as soon as possible and see how it works out in practice.

@wpopielarski
Copy link
Member Author

working on it, hope today will be ready

2016-04-08 11:36 GMT+02:00 Matthias Langer [email protected]:

The only blocking issue left for me is the tests that check for broken
behaviour (see my comment above). Apart from that I think that we should
merge this as soon as possible and see how it works out in practice.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#144 (comment)

1. turns Select/Import qualifiers with `fullNameString` instead of
`nameString`. This makes better recognition of import usage (by types
mainly).
2. ignores some tests for which some imports are promoted to package scope
(it is existing defect).
@wpopielarski
Copy link
Member Author

The last commit doesn't break OrganizeImports when organizing (same as ScalaOverrideIndicatorBuilder in scala-ide reported by Iulian). Not sure that it covers all corners.

@mlangc
Copy link
Collaborator

mlangc commented Apr 8, 2016

Hmm, for me organizing imports on OrganizeImports still breaks; let's fix this in another PR though. LGTM for me.

@fommil
Copy link
Contributor

fommil commented Apr 8, 2016

I look forward to all the ENSIME integration tests failing when you click the merge button 😝

@kiritsuku
Copy link
Member

Good that you said it, I just run the tests and they passed, therefore I can merge. ;)

@kiritsuku kiritsuku merged commit 24d3902 into scala-ide:master Apr 8, 2016
kiritsuku pushed a commit to kiritsuku/scala-ide that referenced this pull request Apr 9, 2016
Adds settings to `OrganizeImportsPreferencePage` to control featue
implemented by PR scala-ide/scala-refactoring#144
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants