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

Too wide matchName for InheritedNewAbstractMethodProblem #196

Closed
patriknw opened this issue Sep 18, 2017 · 5 comments
Closed

Too wide matchName for InheritedNewAbstractMethodProblem #196

patriknw opened this issue Sep 18, 2017 · 5 comments
Labels
Milestone

Comments

@patriknw
Copy link

If one mixin ActorLogging (trait) it correctly complains about InheritedNewAbstractMethodProblem, but the filter to exclude it is:

ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("akka.actor.ActorLogging.akka$actor$ActorLogging$$_log_=")
ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("akka.actor.ActorLogging.log")
ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("akka.actor.ActorLogging.akka$actor$ActorLogging$$_log")

Looks like matchName is for the trait that introduces the problem, instead of the target trait. That means that after adding such filter other breakages of mixing in ActorLogging will not be detected.

@dwijnand
Copy link
Collaborator

/cc @2m?

@2m
Copy link
Contributor

2m commented Sep 27, 2017

For now I have created a failing test-case: #197 Anyone else is free to jump on it if interested.

More information on the issue is in akka/akka#23563 (comment) by @nick-nachos

It is the sbt plugin that needs a fix, as the reporter itself has correct report on this issue:

abstract method foo()Int in interface AA is inherited by class A in new version.

@nick-nachos
Copy link
Contributor

@2m I am not sure what you mean by:

It is the sbt plugin that needs a fix

I have fixed the issue locally by changing this:

case class InheritedNewAbstractMethodProblem(clazz: ClassInfo, inheritedMethod: MemberInfo) extends AbstractMethodProblem(inheritedMethod)

to this:

case class InheritedNewAbstractMethodProblem(clazz: ClassInfo, inheritedMethod: MemberInfo) extends AbstractMethodProblem(new MemberInfo(clazz, inheritedMethod.bytecodeName, inheritedMethod.flags, inheritedMethod.sig))

This fix is in par with the analysis I made in the related akka issue. Would you like to make the change yourself, or should I proceed with a pull request?

@2m
Copy link
Contributor

2m commented Sep 27, 2017

Fix sounds good. Yes, please proceed with a pull request!

@nick-nachos
Copy link
Contributor

nick-nachos commented Sep 27, 2017

Ok great. Before proceeding with this I should note that this change will have a breaking effect to any current consumers of Filters based on the InheritedNewAbstractMethodProblem, which will have to rewrite the corresponding filter expressions. Given that current behavior is considered a bug, I assume that this is not a problem, but I would like your confirmation as well.

To demonstrate with an example, in the scripted test case you provided a current consumer would have to write a filter expression of the form:

ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("T.bar")

which will have to be changed to:

ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("A.bar")

nick-nachos added a commit to nick-nachos/migration-manager that referenced this issue Sep 27, 2017
@2m 2m added this to the 0.1.19 milestone Oct 9, 2017
2m added a commit that referenced this issue Oct 9, 2017
…tchingFix

Match inherited abstract methods against inheriting class #196
@2m 2m closed this as completed Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants