Fix missing SymbolOccurrence for aliased symbol #13609
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fix scala#13556 Previously, we include symbols iff it's name present in source by `namePresentInSource`. However, if the symbol is aliased by, for example, renamed imports, there's difference beteen the name of the symbol and the name in source even though they are the same symbol. What we really need to exclude are the symbols that never appear in source, and the gap between symbol name and name in source is acceptable. Therefore, this commit try to exlucde symbol using `isZeroExtent` instead of `namePresentInSource`
tanishiking
commented
Sep 25, 2021
| @@ -4,11 +4,11 @@ package b | |||
| object Givens/*<-a::b::Givens.*/: | |||
|
|
|||
| extension [A/*<-a::b::Givens.sayHello().[A]*/](any/*<-a::b::Givens.sayHello().(any)*/: A/*->a::b::Givens.sayHello().[A]*/) | |||
| def sayHello/*<-a::b::Givens.sayHello().*/ = s"Hello, I am $any/*->a::b::Givens.sayHello().(any)*/" | |||
| def sayHello/*<-a::b::Givens.sayHello().*/ = s"Hello, I am $any/*->a::b::Givens.sayHello().(any)*/"/*->scala::StringContext#s().*/ | |||
There was a problem hiding this comment.
Introducing StringContext#s is fine, because
- it actually presents in the source
- Scala2 also extracts it https://github.com/scalameta/scalameta/blob/267c5966bc343ee36683ca36bd19cf6e9b825381/tests/jvm/src/test/resources/example/ImplicitConversion.scala#L23
Though the position is a bit wired 🤔
There was a problem hiding this comment.
if i compile
def str = s"abc 123 ${scala.util.Random.nextBoolean}"the span of .s in the desugared code is columns 10-53, so the span comes from somewhere and is truncated, maybe selectSpan or nameSpan?
either way it does not need to be fixed in this PR
tanishiking
commented
Sep 25, 2021
| @@ -1 +1,4 @@ | |||
| import scala.util.control.NonFatal/*->scala::util::control::NonFatal.*/ | |||
| import scala.collection.immutable.{HashMap/*->scala::collection::immutable::HashMap.*//*->scala::collection::immutable::HashMap#*/ as HM} | |||
|
|
|||
| val m/*<-_empty_::Imports$package.m.*/: HM/*->scala::collection::immutable::HashMap#*/[Int/*->scala::Int#*/, Int/*->scala::Int#*/] = HM/*->scala::collection::immutable::HashMap.*/[Int/*->scala::Int#*/, Int/*->scala::Int#*/]() | |||
There was a problem hiding this comment.
Before the change, HM didn't have an occurrence.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix #13556
Previously, we included symbols iff its name present in the source by
namePresentInSourcefef5be0However, if symbols are aliased by, for example, renamed imports, there's a difference between the name of symbol and the name in source, even though they are the same symbol.
We really need to exclude the symbols that never appear in the source and the gap between symbol name and name in the source should be acceptable. Therefore, this commit tries to exclude a symbol using
isZeroExtentinstead ofnamePresentInSourceActually, the diff looks fine.