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

Avoid renaming synthetic occurrences #1077

Merged
merged 5 commits into from Dec 3, 2019
Merged

Avoid renaming synthetic occurrences #1077

merged 5 commits into from Dec 3, 2019

Conversation

@gabro
Copy link
Member

gabro commented Nov 19, 2019

Fixes #1072

Previously we would rename all references of a symbol, including synthetic ones. This resulted in bugs such as #1072. Now we ignore synthetics references when performing a rename.

@gabro gabro requested review from olafurpg and tgodzik Nov 19, 2019
@@ -86,7 +86,8 @@ final class RenameProvider(
txtParams <- if (parentSymbols.isEmpty) List(textParams)
else parentSymbols.map(toTextParams)
currentReferences = referenceProvider.references(
toReferenceParams(txtParams)
toReferenceParams(txtParams),
includeSynthetics = false

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 19, 2019

Member

This will miss references to synthetic .apply, which we do want to rename

This comment has been minimized.

Copy link
@gabro

gabro Nov 19, 2019

Author Member

right, I forgot to "de-focus" the tests locally. I'll think of a solution

This comment has been minimized.

Copy link
@gabro

gabro Nov 19, 2019

Author Member

I've pushed a fix, which is the simplest thing I could think of: decide whether to include or not the synthetics based on the occurrence.

This way we can decide for which symbols we want to include the synthetics, e.g. we do want them for apply

@@ -86,7 +86,8 @@ final class RenameProvider(
txtParams <- if (parentSymbols.isEmpty) List(textParams)
else parentSymbols.map(toTextParams)
currentReferences = referenceProvider.references(
toReferenceParams(txtParams)
toReferenceParams(txtParams),
includeSynthetics = _.symbol.desc.name.value == "apply"

This comment has been minimized.

Copy link
@gabro

gabro Nov 19, 2019

Author Member

Include the synthetics occurrences of apply because we want to rename them, but exclude anything else

@gabro gabro requested a review from olafurpg Nov 20, 2019
Copy link
Collaborator

tgodzik left a comment

Thanks @gabro for fixing this! I haven't really thought about synthetics when implementing rename.

def references(params: ReferenceParams): ReferencesResult = {
def references(
params: ReferenceParams,
includeSynthetics: SymbolOccurrence => Boolean = _ => true

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 22, 2019

Collaborator

Let's do the function as suggested by @olafurpg :

Synthetic => Boolean

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 29, 2019

Collaborator

@olafurpg I took the liberty to change it myself - is this what you meant?

@tgodzik tgodzik force-pushed the gabro:fix-1072 branch from 3ea133b to 86a99b0 Dec 3, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Dec 3, 2019

@olafurpg I rebased the change and this should be ok to merge, what do you think?

@tgodzik
tgodzik approved these changes Dec 3, 2019
@tgodzik tgodzik merged commit d8ed0b4 into scalameta:master Dec 3, 2019
10 checks passed
10 checks passed
ubuntu-latest tests
Details
windows-latest tests
Details
macOS-latest 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
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.