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.

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

Fall back to mtags position occurrences #962

Merged
merged 6 commits into from Oct 6, 2019
Merged

Conversation

@gabro
Copy link
Member

gabro commented Oct 5, 2019

Fixes #961

gabro added 2 commits Oct 5, 2019
@gabro gabro requested review from olafurpg and tgodzik Oct 5, 2019
.allToplevels(source.toInput)
.occurrences
.find(
_.encloses(queryPosition, includeLastCharacter)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 5, 2019

Member

This position doesn’t need to be from edit distance since we parse top levels from the latest text contents of the file.

@tgodzik
tgodzik approved these changes Oct 5, 2019
Copy link
Collaborator

tgodzik left a comment

Awesome! Thanks for finding it and fixing already! 🙏

@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Oct 5, 2019

As discussed offline, I've moved the fallback to ImplementationProvider only, so that it's not in the hot path for definitions.

@gabro gabro requested review from olafurpg and tgodzik Oct 5, 2019
@@ -90,7 +91,13 @@ final class ImplementationProvider(
params,
currentDocument
)
symbolOccurrence <- positionOccurrence.occurrence.toIterable
symbolOccurrence <- {

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 5, 2019

Collaborator

Later when we look for the implementation we load semanticDB for the file and find occurence in textDocument.occurences, can the same situation happen there? If the annotation is on the implementing class?

This comment has been minimized.

Copy link
@gabro

gabro Oct 5, 2019

Author Member

ah, I think so. The occurrence of the sealed trait is not a "definition" anymore in that case (it's a "reference" IIRC). Can you think of a test case for stressing it?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 5, 2019

Collaborator

That could be a modified test case in this PR I think:

    """|/a/src/main/scala/a/Main.scala
       |package a
       |import io.circe.generic.JsonCodec
       |trait Be@@ing
       |@JsonCodec sealed trait <<Animal>> extends Being
       |object Animal {
       |  case object <<Dog>> extends Animal
       |  case object <<Cat>> extends Animal
       |}
       |""".stripMargin

this most likely will not find Animal

This comment has been minimized.

Copy link
@gabro

gabro Oct 5, 2019

Author Member

you're right, I've added the (now failed) test, so that we can work on this tomorrow.

This comment has been minimized.

Copy link
@gabro

gabro Oct 5, 2019

Author Member

ok, I've pushed a fix for that case too. There's indeed a lot of repetion and I hope we can come up with a more harmonized strategy for finding a definition of a symbol in the next days.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 5, 2019

Collaborator

Thanks @gabro !

Copy link
Collaborator

tgodzik left a comment

Just one comment, but I can check it later and fix it.

@gabro gabro dismissed tgodzik’s stale review Oct 5, 2019

we found another bug

gabro added 2 commits Oct 5, 2019
@gabro gabro force-pushed the gabro:fix-961 branch from aab66b2 to 995c18d Oct 5, 2019
@gabro gabro changed the title Fallback to mtags position occurrences Fall back to mtags position occurrences Oct 5, 2019
@tgodzik
tgodzik approved these changes Oct 5, 2019
Copy link
Member

olafurpg left a comment

LGTM 👍 Let's look into this closer during ScalaSphere

@@ -90,7 +91,13 @@ final class ImplementationProvider(
params,
currentDocument
)
symbolOccurrence <- positionOccurrence.occurrence.toIterable
symbolOccurrence <- {
lazy val mtagsOccurrence = Mtags

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 6, 2019

Member
Suggested change
lazy val mtagsOccurrence = Mtags
def mtagsOccurrence = Mtags
@olafurpg olafurpg merged commit df62e8a into scalameta:master Oct 6, 2019
2 checks passed
2 checks passed
build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.