Code action for-comprehension: support overlapping param names #7784
Conversation
A chain of map/flatMap can reuse the same name, e.g. using subsequent `.map(x => x + 1)`. After converting to for-comprehension, those duplicate names need to be renamed. This PR tries to handle some common usages (like pattern matching, anonymous functions), but some are complex enough to justify to not support them, and the code action just gives up. Resolves scalameta#4069
Perl99
force-pushed
the
4069-params-overlap-for-comp
branch
from
cac3c4d to
7c68f7b
Compare
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
Thanks for contributing! Do you have a bit of time to work on it still? We could split the work to simplify the code action first, which might help to do the further work.
|
|
||
| import scala.meta.internal.metals.codeactions.FlatMapToForComprehensionCodeAction | ||
| import scala.meta.internal.metals.codeactions.RewriteBracesParensCodeAction | ||
| import scala.meta.internal.metals.codeactions.{ |
There was a problem hiding this comment.
You need to run sbt scalafixAll which should fix the imports and the scalafix check
tests/unit/src/test/scala/tests/codeactions/FilterMapToCollectCodeActionSuite.scala
Show resolved
Hide resolved
| nameGenerator: MetalsNames, | ||
| ): (List[Enumerator], Option[Term]) = { | ||
| val perhapsValueNameAndNextQual = termApply.args.headOption.flatMap { | ||
| def perhapsValueNameAndNextQual = termApply.args.headOption.flatMap { |
There was a problem hiding this comment.
No, but I think it is a good change - the third case of the code below (case _ => // there is no interesting function in this termApply) does not use this value, so I think it is a good idea to defer walking the remaining tree until needed. The method is called only once.
There was a problem hiding this comment.
lazy val then maybe in case it's used more times in the future?
| ) | ||
| } else | ||
| Enumerator.Val( // when it is map | ||
| Enumerator.Val( // when it is map, |
There was a problem hiding this comment.
| Enumerator.Val( // when it is map, | |
| Enumerator.Val( // when it is map |
| case Term.Function(List(param), term) => | ||
| Some(Pat.Var(Term.Name(param.name.value)), term) | ||
| if (nameGenerator.isNameEncountered(param.name.value)) { | ||
| replaceNameInTermWithNewName(term, nameGenerator, param.name) |
There was a problem hiding this comment.
We would need to do something like (this is just an idea, not really an exact code to use.)
| replaceNameInTermWithNewName(term, nameGenerator, param.name) | |
| compilers.references( | |
| new l.ReferenceParams( | |
| textDocument, | |
| tree.pos.toLsp.getStart(), | |
| new l.ReferenceContext(false), | |
| ), | |
| EmptyCancelToken, | |
| noAdjustRange, | |
| ) |
Looks like we should use compilers.references instead, which can give us semantically correct references to the parameter. We can later check if a given Term.Name position is included in the results.
The compilcation is:
- we would want to do that in resolveCodeAction as we don't want to run it every time we click on a for comprehension, that would require a bit of a refactor. We want to first identify that a code action makes sense here and only then run it.
- compiler.references uses futures, which is fine for resolveCodeAction, but then we need to push Future to all the methods, which becomes complicated.
There was a problem hiding this comment.
This code runs once per map/flatMap. Can compilers.references be used once to find all duplicates in their separate scopes, or it needs to be run multiple times?
If I understand you correctly, method replaceNameInTermWithNewName would still be used, the difference would be detection of naming scopes - instead of nameGenerator.isNameEncountered(param.name.value) probably
There was a problem hiding this comment.
replaceNameInTermWithNewName can just be much simpler, since we only need to search for Term.Name that has a position included in the results
compilers.references would need to be called for each name separately.
|
We could potentially move all the code to resolveCodeAction, but add codeAction method that would just check if we are in aplicable filter.map chain |
2 participants
A chain of map/flatMap can reuse the same name, e.g. using subsequent
.map(x => x + 1). After converting to for-comprehension, those duplicate names need to be renamed.This PR tries to handle some common usages (like pattern matching and anonymous functions), but some are complex enough to justify not supporting them, and the code action just gives up.
See the test cases for supported examples where the renaming works.
Resolves #4069