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 #1431: Avoid rewriting lambdas into ambiguous references #1432

Merged
merged 3 commits into from
Jul 1, 2020

Conversation

carterkozak
Copy link
Contributor

Before this PR

Produced code that did not work in some scenarios.

After this PR

==COMMIT_MSG==
Avoid rewriting lambdas into ambiguous references
==COMMIT_MSG==

Possible downsides?

Note that this uses the expensive SuggestedFixes.compilesWithFix
check which can result in poor performance, particularly on large
projects.
Fortunately this case isn't very common (no occurrences in a
very large internal project).

Note that this uses the expensive `SuggestedFixes.compilesWithFix`
check which can result in poor performance, particularly on large
projects.
Fortunately this case isn't very common (no occurrences in a
very large internal project).
@changelog-app
Copy link

changelog-app bot commented Jun 25, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Avoid rewriting lambdas into ambiguous references

Check the box to generate changelog(s)

  • Generate changelog entry

@@ -170,8 +168,8 @@ private Description convertMethodInvocations(
}

return buildFix(methodSymbol, methodInvocation, root, state, isLocal(methodInvocation))
.map(fix ->
buildDescription(root).setMessage(MESSAGE).addFix(fix).build())
.filter(fix -> SuggestedFixes.compilesWithFix(fix, state))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a quick and dirty fix, we can write something more precise in the future if we find performance suffers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I didn't see a good way to do the ambiguous check on the lambda/method reference receiver, but this would catch it. Should we put this filter in the buildFix method itself to catch any other potential invalid transforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of other paths that result in ambiguous behavior, I'd prefer to catch specific causes so we can add test cases for them, and hopefully write more precise fixes that don't involve recompilation.

@iamdanfox
Copy link
Contributor

// policy?

@carterkozak
Copy link
Contributor Author

policy bot has been sad today.

@carterkozak
Copy link
Contributor Author

🤖 🍿

@bulldozer-bot bulldozer-bot bot merged commit 5be0d9c into develop Jul 1, 2020
@bulldozer-bot bulldozer-bot bot deleted the ckozak/gh1431 branch July 1, 2020 13:02
@svc-autorelease
Copy link
Collaborator

Released 3.32.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants