Skip to content

Ignore added/removed and changed anonymous lambdas for Scala 2.12.x #92

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 12, 2015
Merged

Ignore added/removed and changed anonymous lambdas for Scala 2.12.x #92

merged 2 commits into from
Nov 12, 2015

Conversation

bantonsson
Copy link
Contributor

  • Added checks for the Java ACC_SYNTHETIC flag that means that the method isn't accesible from Java source
  • Added exception for Java ACC_BRIDGE flag, which is set on erased apply methods together with ACC_SYNTHETIC
  • Started using synthetic method name checks, and made them more selective than synthetic field name checks

Fixes #78 (at least the problem described in that ticket)

@bantonsson bantonsson mentioned this pull request Oct 19, 2015
Closed
dotta
dotta reviewed Oct 19, 2015
View reviewed changes
core/src/main/scala/com/typesafe/tools/mima/core/MemberInfo.scala
@@ -7,8 +7,9 @@ object MemberInfo {
/** The index of the string $_setter_$ in this string */
private def setterIdx(name: String) = name.indexOf(setterTag)

private val setterTag = "$_setter_$"
private val setterTag = "$_setter_"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't remember the exact failing test now but there was something that didn't match with it. Maybe it's by now just a remnant of an earlier attempt. I'll try to back it out later, to minimize the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. I'm simply curious :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good that you are. It's better not to introduce changes in behavior unless there is a need for it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm investigating this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, this is amusing. because the method in the test is specifically named bar, that's somehow being interpreted as a | character name-mangled as $bar. heh

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, there's already an issue on this, it's #50 (@retronym helped me find it). I'll comment there.

in any case, it's actually irrelevant to this PR now, Bjorn removed the change in question in the latest version.

@dotta
Copy link
Contributor

dotta commented Oct 19, 2015

@bantonsson Really nice! I guess now it's on @SethTisue (or someone else in the Scala team) to update Scala so that lambdas are generated with the ACC_SYNTHETIC flag.

@SethTisue
Copy link
Collaborator

it appears to me that scalac already generates lambdas with ACC_SYNTHETIC in both 2.12.0-M2 and 2.12.0-M3. (I'm not sure where we got the opposite impression...!) /cc @retronym

@bantonsson so that explains why the tests you wrote are already passing! and why they still pass even if I take out your "$anonfun$"-based checking. (as a sanity check, I made sure that if I also remove the !isSynthetic check, the tests fail, as expected)

@SethTisue SethTisue self-assigned this Nov 10, 2015
@bantonsson bantonsson mentioned this pull request Nov 10, 2015
Closed
@bantonsson
Copy link
Contributor Author

@SethTisue Thanks a lot for clearing up this confusion. I have no idea how we missed the ACC_SYNTHETIC flag when we looked at it the first time. It's clearly there when I decompile the 2.12.0-M3 classes now.

@dotta I'll try to clean up this PR, and fix #36 as well.

@dotta
Copy link
Contributor

dotta commented Nov 10, 2015

@bantonsson Excellent!

@bantonsson
Ignore added/removed and changed anonymous lambdas for Scala 2.12.x
25b6ed2 
* Added checks for the Java ACC_SYNTHETIC flag that means that the method isn't accesible from Java source
* Added exception for Java ACC_BRIDGE flag, which is set on erased apply methods together with ACC_SYNTHETIC
@bantonsson
Don't report synthetic erased bridge methods (e.g. apply) lightbend-l…
ec87be9 
…abs#36

The correctly typed apply method is already reported so don't report the erased method as well.
@bantonsson bantonsson force-pushed the wip-ban-add-test-for-anonymous-lambdas branch from 29a1d52 to ec87be9 Compare November 10, 2015 09:06
SethTisue added a commit that referenced this pull request Nov 12, 2015
@SethTisue
Merge pull request #92 from bantonsson/wip-ban-add-test-for-anonymous…
6ac3bc1 
…-lambdas

Ignore added/removed and changed anonymous lambdas for Scala 2.12.x
@SethTisue SethTisue merged commit 6ac3bc1 into lightbend-labs:master Nov 12, 2015
@SethTisue
Copy link
Collaborator

really appreciate y'all tackling this

@SethTisue SethTisue mentioned this pull request Nov 12, 2015
Closed
@adriaanm
Copy link
Contributor

🌈

@SethTisue SethTisue added this to the 0.1.8 milestone Nov 12, 2015
This was referenced Jan 10, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SethTisue SethTisue

Labels
None yet
Milestone
0.1.8
Development

Successfully merging this pull request may close these issues.
4 participants
@bantonsson @dotta @SethTisue @adriaanm