WIP: Issue 29 Xor #35
Conversation
| case t @ DType(t"cats.data.Xor") => t -> xor | ||
| case t @ DType(t"cats.data.Xor.Left") => t -> xorLeft | ||
| case t @ DType(t"cats.data.Xor.Right") => t -> xorRight | ||
| } |
There was a problem hiding this comment.
Here I am playing around with the idea of collecting interesting nodes and information about them and generating patches later. It doesn't add much in this case but I thought it could be useful if we attempted to do some sort of fusion later, for example the same tree might have multiple sets of appends performed on it, in which case we could merge them.
| val firstImport = allImports.head._1 | ||
| val firstImportFirstToken = firstImport.tokens.head | ||
| val tokenBeforeFirstImport = | ||
| ast.tokens.takeWhile(_ != firstImportFirstToken).lastOption |
There was a problem hiding this comment.
This is terrible. Anyone have ideas for better ways to find the top level import block? Currently I just grab the first, then the token before the first as I found that if the first becomes deleted (Patched with "") then I can not insert before it.
There was a problem hiding this comment.
I need to figure out what to do if there are no imports, and also what to do if there is a wildcard import.
There was a problem hiding this comment.
I also need to be smarter about existing imports in general. Since I am collecting up all imports and patches, I could just find imports needed per patch that are not already imported. Maybe I need to switch to a structure with more information than just patch to make that convenient.
There was a problem hiding this comment.
See comment below on imports.
| val removedImports = partitionedImports.collect { | ||
| case (t, (remove, keep)) if keep.isEmpty => Seq(Patch.delete(t)) | ||
| case (t, (remove, keep)) => remove.map(Patch.delete) | ||
| }.flatten |
There was a problem hiding this comment.
This is a better example of where merging actions on a tree may be useful. I was considering the example where I had separated Xor2Either and XorT2EitherT into two rewrites, in that case if I collected the removals of imports I may see that all importees below a given importer are removed, and then remove the whole importer in one shot. Again, not really sure if this is worth the complexity yet.
There was a problem hiding this comment.
Imports are still an open question, and they will require semantic information to handle hairy cases like relative imports etc. We had a brief discussion on gitter https://gitter.im/scalacenter/scalafix?at=58526907058ca967374c48b3
I think that imports will eventually be handled by a custom import rewrite. That rewrite will probably take liberty to force a particular import style
import a.b
import a.c
// OR
import a.{b, c}Users might need to run the import rewrite once before running other scalafix rewrites in order to minimize the diff. Similar to formatting.
I suggest you don't worry too much about imports for now. Just let them be.
| def insertAfter(tree: Tree, toAppend: String): Patch = | ||
| replace(tree, s"${tree.syntax}$toAppend") | ||
|
|
||
| def delete(tree: Tree): Patch = replace(tree, "") |
There was a problem hiding this comment.
These builders feel useful but I need to find a better way to insert before and after without modifying the actual tree as I am currently doing. With this definition if I insertBefore a tree, then later replace the tree, the insertBefore is lost.
There was a problem hiding this comment.
These are useful indeed. I had thought of making Patch an adt of Add/Delete/Replace operations. The Add and Delete operations could resolve conflicts on patches for the same token, but care needs to be taken for the order of things.
There was a problem hiding this comment.
I've discussed with Eugene about merging Patch into scalameta/scalameta once we've figured out the details on how to make it robust and useful
There was a problem hiding this comment.
Looks like a great start! Imports are indeed the big problem and I suggest we wait with them for now. I will probably need to look at them myself for ExplicitImplicit.
FYI, I'm on away on vacation in Iceland for the holidays, so I may be slow to respond until January 7th.
| def insertAfter(tree: Tree, toAppend: String): Patch = | ||
| replace(tree, s"${tree.syntax}$toAppend") | ||
|
|
||
| def delete(tree: Tree): Patch = replace(tree, "") |
There was a problem hiding this comment.
These are useful indeed. I had thought of making Patch an adt of Add/Delete/Replace operations. The Add and Delete operations could resolve conflicts on patches for the same token, but care needs to be taken for the order of things.
| def insertAfter(tree: Tree, toAppend: String): Patch = | ||
| replace(tree, s"${tree.syntax}$toAppend") | ||
|
|
||
| def delete(tree: Tree): Patch = replace(tree, "") |
There was a problem hiding this comment.
I've discussed with Eugene about merging Patch into scalameta/scalameta once we've figured out the details on how to make it robust and useful
| val removedImports = partitionedImports.collect { | ||
| case (t, (remove, keep)) if keep.isEmpty => Seq(Patch.delete(t)) | ||
| case (t, (remove, keep)) => remove.map(Patch.delete) | ||
| }.flatten |
There was a problem hiding this comment.
Imports are still an open question, and they will require semantic information to handle hairy cases like relative imports etc. We had a brief discussion on gitter https://gitter.im/scalacenter/scalafix?at=58526907058ca967374c48b3
I think that imports will eventually be handled by a custom import rewrite. That rewrite will probably take liberty to force a particular import style
import a.b
import a.c
// OR
import a.{b, c}Users might need to run the import rewrite once before running other scalafix rewrites in order to minimize the diff. Similar to formatting.
I suggest you don't worry too much about imports for now. Just let them be.
| val firstImport = allImports.head._1 | ||
| val firstImportFirstToken = firstImport.tokens.head | ||
| val tokenBeforeFirstImport = | ||
| ast.tokens.takeWhile(_ != firstImportFirstToken).lastOption |
There was a problem hiding this comment.
See comment below on imports.
|
|
||
| //experimenting to see if I can create lists of types to try to match. No luck so far. | ||
| val replacementTypes: List[ReplaceType] = | ||
| List(xorT, xor, xorLeft, xorRight) |
There was a problem hiding this comment.
I think you can use the unapply method on Type nodes
| val xorLeft: ReplaceType = | ||
| ReplaceType(t"cats.data.Xor.Left", t"scala.util.Left", "Left") | ||
| val xorRight: ReplaceType = | ||
| ReplaceType(t"cats.data.Xor.Right", t"scala.util.Either.Right", "Right") |
There was a problem hiding this comment.
Down the road, we may be able to generalize rewrites so they can be implement from a Seq[(Tree, Tree)]. Very exciting.
| val termReplacements = methodChanger.gatherPatches(replacementTerms) | ||
| val addedImports = importAdder.gatherPatches(additionalImports) | ||
|
|
||
| addedImports ++ typeReplacements ++ termReplacements |
There was a problem hiding this comment.
This class is now a rough approximation of what I think a nice api for creating type refactorings might look like. List some types to replace, list some methods to replace, and either derive new imports needed or list those as well.
| "cats.data.EitherT", | ||
| "cats.implicits._", | ||
| "scala.util.Either" | ||
| ) |
There was a problem hiding this comment.
I ran out of time tonight to create more strongly typed versions of imports.
There was a problem hiding this comment.
These can be written as q"import cats.data.EitherT", but I don't know if that's any better.
| ReplaceType(t"cats.data.Xor", t"scala.util.Either", "Either"), | ||
| ReplaceType(t"cats.data.Xor.Left", t"scala.util.Left", "Left"), | ||
| ReplaceType(t"cats.data.Xor.Right", t"scala.util.Either.Right", "Right") | ||
| ) |
There was a problem hiding this comment.
I will work on removing the final string in ReplaceType and just pull it out of the new type to be used.
There was a problem hiding this comment.
Could we extract the last string like this?
@ t"scala.util.Either.Right" match {
case t"$_.$name" => name
}
res3: Type.Name = RightThere was a problem hiding this comment.
Any ideas how I would pull "Right" out of "scala.util.Right.apply"? I am stumped, spent a few hours on it tonight and getting nowhere.
Quasiquotes are confusing me. I can pull the prefix from t"cats.data.Xor.Right.apply" using t"$prefix.$" (cats.data.Xor.Right) and I can pull the name from t"cats.data.Xor.Right" using t"$.$name" but I cannot pull the prefix out, then pull the name from it (I get a match error) and also can't combine them as in t"$preprefix.$prefixname.$_" ? I'm trying to do things the "right" way but keep running into issues that take hours with quasiquotes and would take a few minutes with string splitting.
There was a problem hiding this comment.
The biggest issue being, now the heck to I make them compose? Say I used some sort of path to represent "scala.util.Right.apply". With /scala/util/Right/apply it's trivial to get the name, the fully qualified name, the parent, the parents name alone, or full path, etc. And if I make a function to get "name" which just does path.takeRight(1) and another to get parent which is path.init, it's easy to compose them and get parent's name. With partial functions, quasiquotes, and pattern matching I am finding the few bits of scala syntax I have run into that really resist composition. Pointers hugely appreciated.
There was a problem hiding this comment.
I'm sad to hear this is a productivity killer.
You don't have to use quasiquotes. I'm guilty of using strings to strip of a prefix package name just because I just wanted to see something work, see here. However, string manipulation is almost like programming with void pointers in C, try to avoid it when possible.
An alternative to quasiquotes that's not string manipulation is using the regular Tree ADT. I do this myself all the time (example). Quasiquotes tend to hide the fact that you're working with tree structures. With scalafmt, I found the relationship counter-intuitive between the code on the surface and the underlying parsed AST, for example
@ q"a.b.c.d".structure
res20: String = """
Term.Select(Term.Select(Term.Select(Term.Name("a"), Term.Name("b")), Term.Name("c")), Term.Name("d"))
"""
// note `d` is at the top and `a` is at the bottomIf you want to use quasiquotes, you might be looking for transform in combination with StructurallyEqual or .structure
@ val prefix = q"a.b.c"
prefix: Term.Select = a.b.c
@ val fqn = q"$prefix.d.e"
fqn: Term.Select = a.b.c.d.e
@ fqn.transform {
case q"$somePrefix.$name" if somePrefix.structure == prefix.structure => name
}
res15: Tree = d.e
| : PartialFunction[m.Tree, (scala.meta.Term, ReplaceTerm)] = { | ||
| case t @ DTerm(desugared) | ||
| if desugared.syntax.startsWith(rt.original.syntax) && | ||
| desugared.syntax.endsWith("]") => |
There was a problem hiding this comment.
I had to resort to string matching here. If anyone has better ideas I would love the feedback.
There was a problem hiding this comment.
Are you doing this to overcome the inserted .apply[T] in the desugared output? I matched on the .apply in the rewrite: https://github.com/olafurpg/scalafix/blob/445b2cd783ef9d1e041198663ed335747133dfcc/core/src/main/scala/scalafix/rewrite/Xor2Either.scala#L22
Maybe I was too optimistic about the capabilities of this desugared approach. I am afraid this desugared approach will always require awkward workarounds like this.
In the future, I'm aiming for a user experience where the user can configure in .scalafix.conf
rename = [
{ from = "cats.data.Xor", to = "scala.util.Either }
]
and everything just works™️. But I suspect we'll need to get type equality =:= in the semantic api to do that.
| def partialTypeMatch( | ||
| rt: ReplaceType): PartialFunction[m.Tree, (m.Type, ReplaceType)] = { | ||
| case t @ DType(desugared) | ||
| if StructurallyEqual(desugared, rt.original).isRight => |
There was a problem hiding this comment.
I was able to use StructurallyEqual for types, but haven't figured out how to do it for methods yet without losing the arguments when I create patches. I end up matching both the method, and the method application with args, which overwrites them when I would like to not match on application.
|
|
||
| sealed abstract class Xor[+A, +B] extends Product with Serializable | ||
| sealed abstract class Xor[+A, +B] extends Product with Serializable { |
There was a problem hiding this comment.
Stub out enough of cats.data.Xor and XorT interface to be able to test rewrites without needing to add a dependency on cats
|
|
||
| import scala.language.implicitConversions | ||
|
|
||
| package object implicits { |
There was a problem hiding this comment.
More stubbing of cats interfaces for testing purposes.
|
|
||
| val typeReplacements = typeChanger.gatherPatches(replacementTypes) | ||
| val termReplacements = methodChanger.gatherPatches(replacementTerms) | ||
| val addedImports = importAdder.gatherPatches(additionalImports) |
There was a problem hiding this comment.
I am not removing imports yet. I plan to work on this later if @olafurpg doesn't beat me to it.
There was a problem hiding this comment.
Great job, I'm really excited to see progress on this. Some comments:
- Have you tried running this on actual projects like circe <v0.5? You need to run
very publishLocaland then you can test either manually (clone repo and editproject/plugins.sbtor automatically withscalafix-tests/testOnly <ProjectName>(see IntegrationPropertyTest.scala). You can poke into the repository intarget/it/circeand see diffs withgit diff - Can you please squash my WIP commit
- If you feel this is ready to be merged, then I'm happy to give it a LGTM 👍 and publish 0.2.1 with this new rewrite.
- I'm gonna go ahead and give you collaborator rights so you're not blocked by me. I'm currently on holiday in Iceland so I may be slow to respond, I'll do my best to help.
| } | ||
|
|
||
| //This is currently a very dumb implementation. | ||
| //It does no checking for existing imports and makes |
There was a problem hiding this comment.
As discussed, I think this is OK while we're learning the ropes with these rewrites. It's better to add too many imports than leave out missing imports. Users can run "Organize Imports" in their IDE to clean up unused imports.
| "cats.data.EitherT", | ||
| "cats.implicits._", | ||
| "scala.util.Either" | ||
| ) |
There was a problem hiding this comment.
These can be written as q"import cats.data.EitherT", but I don't know if that's any better.
| case object Xor2Either extends Rewrite { | ||
| override def rewrite(ast: m.Tree, ctx: RewriteCtx): Seq[Patch] = { | ||
| implicit val semanticApi: SemanticApi = getSemanticApi(ctx) | ||
| object DType extends Desugared[Type] |
There was a problem hiding this comment.
I think we can move these to the SemanticApi trait instead of copying them around.
| ReplaceType(t"cats.data.Xor", t"scala.util.Either", "Either"), | ||
| ReplaceType(t"cats.data.Xor.Left", t"scala.util.Left", "Left"), | ||
| ReplaceType(t"cats.data.Xor.Right", t"scala.util.Either.Right", "Right") | ||
| ) |
There was a problem hiding this comment.
Could we extract the last string like this?
@ t"scala.util.Either.Right" match {
case t"$_.$name" => name
}
res3: Type.Name = Right|
|
||
| case class ReplaceTerm(original: m.Type, newString: String) | ||
|
|
||
| class ChangeMethod(ast: m.Tree, ctx: RewriteCtx)(implicit sApi: SemanticApi) { |
There was a problem hiding this comment.
are you using ctx? btw, the semantic api is part of ctx
There was a problem hiding this comment.
Probably not. I will double check.
| : PartialFunction[m.Tree, (m.Type, ReplaceType)] = | ||
| replacementTypes.map(partialTypeMatch).reduce(_ orElse _) | ||
|
|
||
| // NOTE. This approach is super inefficient, since we run semantic.desugar on |
There was a problem hiding this comment.
Ditto, this is really inefficient. But I say we don't worry about it until it's a problem.
| val t: Either[Int, String] = r.map(_ + "!") | ||
| val nest: Seq[Either[Int, Either[String, Int]]] | ||
| val u: EitherT[Future, Int, String] = ??? | ||
| } |
There was a problem hiding this comment.
What about for comprehensions? Is it necessary to add .right on Xor.flatmap or does cats.implicits pimp up Either?
There was a problem hiding this comment.
flatMap and map it turns out are going to be a problem. I have run on a few external repositories but it is not ready to merge, I need to handle imports better and more importantly I need to deal with the differences in variance between Either.map/flatMap and Xor.map/flatMap. Do to issues in the left side variance, In some cases I will need to replace them with pattern matching. Not looking forward to it but I don't believe there is another solution.
|
There are merge conflicts and I am not able to successfully resolve them yet for reasons I don't yet understand. My current theory is that the desugar implementation might have changed, I am no longer able to match anything in the desugaring step from what I can see. |
|
@olafurpg I tried to do the squash you requested but seem to have messed it up and cannot yet figure out how. I will be leaving town on vacation soon, if I have a chance I will try to get to this but it may get neglected for a while. |
|
I don't know the fix yet but it looks like the Desugar api is broken. Nothing matches, if i turn on the logging statement in Desugar it only returns: |
|
I experimented with simplifying the I'm sorry but I'm afraid I don't time to take a closer look at this until next week when I return to work. I have a feeling we may be able to add a Until then, Merry Christmas and a Happy New Year! 🎅 |
|
Got the merge figured out, it had nothing to do with the changes to Desugared, desugared wasn't working because I messed up the merge and lost the scalacOption -Yrangedpos which desugaring relies on. |
|
Aah, that makes sense. Yes, the current preliminary semantic api in scala.meta (which is mostly copy-paste from scalafix) asserts that yrangepos is enabled in settings: https://github.com/scalameta/paradise/blob/281484d6f5f06575af2139e00e871643af8268f4/plugin/src/main/scala/org/scalameta/paradise/mirrors/Mirrors.scala#L123 |
|
Added Circe test. I am not able to get this to even run for me locally, it always passes with no changes. Will keep working on it but pushed it to see if the CI system would run it correctly. |
|
Seems a bug in integrations was the reason why there were no diffs. Fix in #37 |
|
I am closing this pr. It was useful to investigate the experience of doing code rewrites with a patch based mechanism and I have now done enough to have learned I do not want to continue with this approach. I will try the exercise again using either a transform or prism based approach next. |
|
This pr has been a valuable learning experience for me. Thank you for going
trying out this out at such an early stage. I agree, we need a higher level
patch api. I am drafting the design in my head as we speak and I hope to
implement it as soon as my right hand recovers.
…On Wed, 11 Jan 2017 at 00:44, Shane Delmore ***@***.***> wrote:
I am closing this pr. It was useful to investigate the experience of doing
code rewrites with a patch based mechanism and I have now done enough to
have learned I do not want to continue with this approach. I will try the
exercise again using either a transform or prism based approach next.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#35 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABV8Xa6oyEtI1Wv4CkD8NWj5Kaipt76Cks5rRBfHgaJpZM4LPwW7>
.
|
This isn't even close to ready yet, I am just making a pr to get some visibility and feedback on early trials.