Skip to content

Internally generated interim variables/fields from inline keyword + package private create collision with trait mixin #18646

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

Open
mdedetrich opened this issue Oct 4, 2023 · 8 comments

Comments

@mdedetrich
Copy link

mdedetrich commented Oct 4, 2023

Compiler version

3.3.1

Minimized code

I am in the process of trying to minimize the code There is a minimisation at https://github.com/mdedetrich/scala3-inline-temporary-variable-collision. Critical parts of the original codebase are

Output

You can look at the error here (https://github.com/apache/incubator-pekko/actions/runs/6403063454/job/17380823553), i.e.

[error] -- [E164] Declaration Error: /home/runner/work/incubator-pekko/incubator-pekko/actor/src/main/scala/org/apache/pekko/actor/ActorCell.scala:420:21 
[error] 420 |private[pekko] class ActorCell(
[error]     |                     ^
[error]     |error overriding method inline$Unsafe$i1 in trait ChildrenInline of type (x$0: org.apache.pekko.util): object org.apache.pekko.util.Unsafe;
[error]     |  method inline$Unsafe$i1 in trait DispatchInline of type (x$0: org.apache.pekko.util): object org.apache.pekko.util.Unsafe class ActorCell inherits conflicting members:
[error]     |  method inline$Unsafe$i1 in trait ChildrenInline of type (x$0: org.apache.pekko.util): org.apache.pekko.util.Unsafe  and
[error]     |  method inline$Unsafe$i1 in trait DispatchInline of type (x$0: org.apache.pekko.util): org.apache.pekko.util.Unsafe
[error]     |(Note: this can be resolved by declaring an override in class ActorCell.)
[error] one error found

Expectation

That the code compiles. This is definitely due to inline i.e. if you remove the inline keyword then the codebase compiles fine (actually only need to remove one of the inlines for the code to compile and it doesn't matter which one which gives more credence to the interim variable collision theory) . From the surface it looks like scala3 is generating interim variables/fields (due to inline) which happen to have the exact same name (i.e. inline$Unsafe$i1) in multiple different traits that is causing a collision and its this resulting collision which causes a spurious "please override one of the fields" (which is not even possible because they are internally generated, i.e. not in the source code).

@mdedetrich mdedetrich added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 4, 2023
@mdedetrich mdedetrich changed the title Internally generated interim variables/fields from inline keyword create collision with trait mixin Internally generated interim variables/fields from inline keyword + package private create collision with trait mixin Oct 6, 2023
@mdedetrich
Copy link
Author

mdedetrich commented Oct 6, 2023

So I have some good news, I managed to found out the offending culprit which is package private, i.e. if I remove private[pekko] from https://github.com/mdedetrich/incubator-pekko/blob/732615b4679a4cded147daf337937878e65336ba/actor/src/main/scala/org/apache/pekko/util/Unsafe.scala#L22C1-L22C1 then the code compiles fine.

It seems that the usage of package private is somehow altering the variables being generated which is causing the collision?

Still trying to create a working minimization but hopefully since I figured out the cause it should be easier.

@mdedetrich
Copy link
Author

I have managed to successfully minimize the issue, please see https://github.com/mdedetrich/scala3-inline-temporary-variable-collision

@bishabosha bishabosha added area:refchecks area:inline and removed stat:needs triage Every issue needs to have an "area" and "itype" label area:refchecks labels Oct 16, 2023
@nicolasstucki
Copy link
Contributor

Minimization

package pkg
trait A {
  inline final def aInline: String = pkg.nest.Obj.field
}
class APlusB extends A with B
package nest {
  private[pkg] object Obj {
    var field: String = ""
  }
}
package pkg
trait B {
  inline final def bInline: String = pkg.nest.Obj.field
}

@nicolasstucki
Copy link
Contributor

Same root cause as in #13215. The accessor is to a static member is generated in the class instead of a static scope.

Furthermore, these accessors use a mangled name that does not contain the the prefix of the class (inline$A$Obj$i1) to disambiguate for which trait we are generating this local copy of the accessor.

We also need 2 files to make the local index of the generated accessor be the same ($i1). If these are in the same file we do not have an issue.

@nicolasstucki
Copy link
Contributor

Using the -WunstableInlineAccessors from scala/improvement-proposals#58 would catch this issue and suggest the use of @publicInBinary to avoid the creation of the accessor.

@nicolasstucki
Copy link
Contributor

We cannot fix this collision without causing binary incompatibilities. We need -WunstableInlineAccessors before we can consider fixing this issue.

To fix this issue we need to generate this accessor with the full owner prefix in it. It would be good to additionally make it static.

@mdedetrich
Copy link
Author

To fix this issue we need to generate this accessor with the full owner prefix in it. It would be good to additionally make it static.

Thanks, so for our case its not critical since we don't have to use private[pekko] (we already use @InternalApiOnly and beforehand it was a java class) unless there happens to be some other easy workaround (which I can't see at the moment).

We cannot fix this collision without causing binary incompatibilities. We need -WunstableInlineAccessors before we can consider fixing this issue.

So does that mean fixing this core problem is out of the question for Scala 3.3.x series?

@nicolasstucki
Copy link
Contributor

So does that mean fixing this core problem is out of the question for Scala 3.3.x series?

Unfortunately, yes

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

No branches or pull requests

3 participants