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

Fix renames in case of macro annotations #1095

Merged
merged 2 commits into from Dec 3, 2019

Conversation

@tgodzik
Copy link
Collaborator

tgodzik commented Dec 1, 2019

Previously, renames would not work correctly in case of macro annotations - it would not find annotated classes definition and create an additional reference pointing to the annotation itself.

Now, we always try to find the definition separately for non local symbols and we check all references to be at the right names.

@tgodzik tgodzik requested a review from olafurpg Dec 1, 2019
@tgodzik tgodzik force-pushed the tgodzik:fix-macro-issues branch from 3e20c32 to 783fa61 Dec 1, 2019
Copy link
Member

olafurpg left a comment

A few clarifying questions, I haven't reviewed the code in detail yet. It would be great if we can handle macro annotation definitions!

// we can't get definition by name for local symbols
toReferenceParams(txtParams, includeDeclaration = isLocal),
// local symbol will not contain a proper name
strictCheck = !isLocal

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 1, 2019

Member

Can you clarify what strictCheck = true does?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 1, 2019

Author Collaborator

I added a check that looks into what is underneath a range - anything there should be contained in the symbol itself. So string Animal should be contained in symbol a/Animal#

Not the best solution, but I haven't figured out how else to filter out the odd reference under @JsonCodec

checkStrict might not be the best name 🤔

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 1, 2019

Author Collaborator

Also I do the check only for renames - I think it's better to sacrifice a it of performance for correctness.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 2, 2019

Member

Sounds like a good idea! How does this play with token edit distance? I thought we already kind of do something like this 🤔 Otherwise, how about renaming strictCheck into checkMatchesText?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 2, 2019

Author Collaborator

Sure, I will also check with the edit distances - might not work currently because of that

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 2, 2019

Author Collaborator

I checked - token distance is not an issue since we use the reference range to find the name in the snapshot. Token distance is taken into account later on.

This is also tested in the RenameLspSuite since we simulate an edit there.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 2, 2019

Author Collaborator

Renamed to checkMatchesText

|package a
|import io.circe.generic.JsonCodec
|trait LivingBeing
|@JsonCodec sealed trait <<An@@imal>> extends LivingBeing

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 1, 2019

Member

What happens if you rename an Animal reference from another file?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 1, 2019

Author Collaborator

That works - I can add another test for that.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 2, 2019

Member

It would be good to add a test for this I think just in case.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 2, 2019

Author Collaborator

Added another case, fortunately works 😅

Copy link
Member

olafurpg left a comment

LGTM 👍 Very glad to see this fixed 😄

|package a
|import io.circe.generic.JsonCodec
|trait LivingBeing
|@JsonCodec sealed trait <<An@@imal>> extends LivingBeing

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 2, 2019

Member

It would be good to add a test for this I think just in case.

@tgodzik tgodzik requested a review from sswistun-vl Dec 3, 2019
@tgodzik tgodzik merged commit 0b266d5 into scalameta:master Dec 3, 2019
9 checks passed
9 checks passed
Windows unit tests
Details
Linux unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
@tgodzik tgodzik deleted the tgodzik:fix-macro-issues branch Dec 3, 2019
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.