Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Experimenting with scala.meta mirror #57

Merged
merged 5 commits into from
Feb 10, 2017
Merged

Experimenting with scala.meta mirror #57

merged 5 commits into from
Feb 10, 2017

Conversation

olafurpg
Copy link
Contributor

@olafurpg olafurpg commented Feb 9, 2017 •
edited
Loading

This PR integrates the scala.meta mirror even more tightly into scalafix. It includes an example Xor2Either rewrite using the Symbol/Signature API.

cc/ #29 @ShaneDelmore

@jvican
Copy link

jvican commented Feb 9, 2017

👍 🎉

@olafurpg olafurpg force-pushed the scala.meta-mirror branch 3 times, most recently from 90228ba to f7f1f67 Compare February 9, 2017 18:06
xeno-by
xeno-by reviewed Feb 10, 2017
View reviewed changes
core/src/main/scala/scalafix/rewrite/Xor2Either.scala
Replace(Symbol("_root_.cats.data.XorFunctions.left."), q"Left"),
Replace(Symbol("_root_.cats.data.XorFunctions.right."), q"Right"),
RemoveGlobalImport(importer"cats.data.Xor"),
RemoveGlobalImport(importer"cats.data.XorT")
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. This looks amazingly concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! This is pretty amazing. And it can easily be parsed from config files, making it easy for library authors to ship custom (simple) rewrites

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also make this interactive, i.e have a CLI app with a prompt where you can enter your refactorings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be doable. It would be cool to offer a preview of the diff of a single rewrite, so that you can reuse the same semantic db to run multiple rewrites.

Copy link
Contributor

Choose a reason for hiding this comment

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

Next step, let's tie this into the @deprecated annotation. If my project has scalafix enabled, and I use a lib that has annotated a method with @deprecated while pointing it to a scalafix rewrite bundled with it, then offer to run the rewrite to remove the deprecated usage.

core/src/main/scala/scalafix/syntax/package.scala
Type.Select(qual, Type.Name(name))
case _ => ref.syntax.parse[Type].get.asInstanceOf[Type.Ref]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, but I was using it when I implemented organise imports with Mirror. This PR didn't manage to do the full transition.

core/src/main/scala/scalafix/syntax/package.scala
implicit class XtensionSymbol(symbol: Symbol) {
/** Returns simplified version of this Symbol.
*
* - No Symbol.Multi
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are other non-trivial symbols in the multi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then you need to write a custom rewrite. I made this normalized symbol to simplify the interface for authors writing basic rewrites, basically so it can be done via simple config file.

core/src/main/scala/scalafix/syntax/package.scala
}
tree.asInstanceOf[T] // should crash
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we add Symbol.name and Symbol.fullName, where the former always returns an XXX.Name and the latter does what you implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be perfect 👍 I opened a ticket in scala.meta scalameta/scalameta#675

project/Build.scala Outdated
@@ -5,12 +5,13 @@ style = defaultWithAlign
}*/

object Build {
val scalametaV = "1.6.0-641"
val scalametaV = "1.6.0-652.1486659769696"
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this version do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Configurable fix for scalameta/scalameta#666

core/src/main/scala/scalafix/util/TreeExtractors.scala
def unapply(tree: Tree): Option[(Tree, Tree)] =
tree.parent.map(parent => tree -> parent)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using it, and I have a similar one in scalafmt. Should probably be in contrib.

@olafurpg olafurpg mentioned this pull request Feb 10, 2017
Closed
@olafurpg
Clean up PR
76c1d8e
@olafurpg
Track down performance regression in integration tests.
bf08436 
Doing .symbol on every Ref in a Traverser is apparently really slow.
This provides a valuable lesson: scalafix should not run rewrites during
compilation, scalafix should run rewrites against a semanticdb.
Why? By dumping the semanticdb first, we compile as fast as possible AND
it allows us to run rewrites on multiple compilation units in parallel.
@olafurpg
Copy link
Contributor Author

olafurpg commented Feb 10, 2017 •
edited
Loading

I tracked down a performance issue with running the Replace patches on slick. The CI was taking >1h when it should complete in 5min. It appears that calling .symbol on every Ref in a tree is slow.

This provides a valuable lesson: scalafix should not run rewrites during compilation, scalafix should run rewrites against a pre-made semanticdb. Why?

  • simpler fixture for testing rewrites, load a Mirror and go!
  • easier to experiment with semantic rewrites in worksheets
  • easier to iterate on rewrite implementations, no need to re-compile large projects (slick/circe) all the time
  • compilation is faster because we don't run rewrites
  • rewrites can run in parallel! (big win for large projects)

@olafurpg olafurpg merged commit 402dd8f into scalacenter:master Feb 10, 2017
@olafurpg olafurpg deleted the scala.meta-mirror branch February 10, 2017 19:17
@olafurpg olafurpg mentioned this pull request Feb 13, 2017
Closed
bjaglin pushed a commit to liancheng/scalafix that referenced this pull request May 23, 2023
@github-brice-jaglin
importsOrder: correct comparison with IntelliJ in doc (scalacenter#57)
5eeaa74
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xeno-by xeno-by xeno-by left review comments

@ShaneDelmore ShaneDelmore ShaneDelmore left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@olafurpg @jvican @xeno-by @ShaneDelmore