Skip to content

Generate static inline accessors in the outermost scope #16796

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

Closed
wants to merge 2 commits into from

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Feb 1, 2023

If a class C needs inline accessors that would be added top-level or if the accessor is to a static member, we place it in a new invisible module C$inline$accessors.

If the accessor location in the new scheme is not the same as the previous location, we also generate the old accessor for backward binary compatibility but do not use it.

Fixes #13215
Fixes #15413

Example

package foo
class C:
  inline def baz: Unit = D.bazImpl
object D:
  private[foo] def bazImpl: Unit = ()

becomes

package foo
class C:
  inline def baz: Unit = foo.C$inline$accessors.inline$bazImpl:Unit
  // old accessor
  def inline$bazImpl$i1(x$0: foo.D): Unit = x$0.bazImpl
 
// new static accessors
object C$inline$accessors: 
  def inline$bazImpl: Unit = foo.D.bazImpl

object D:
  private[foo] def bazImpl: Unit = ()

@nicolasstucki nicolasstucki self-assigned this Feb 1, 2023
@sjrd
Copy link
Member

sjrd commented Feb 1, 2023

This ticket for this bug raised questions about binary compatibility. Is this fix actually compatible?

@nicolasstucki nicolasstucki force-pushed the fix-inline-accessors branch 3 times, most recently from d3f782e to f25bec2 Compare February 1, 2023 09:15
@nicolasstucki
Copy link
Contributor Author

Yes, there are some issues with binary compatibility in this first implementation. I plan to generate the old (sometimes buggy) accessors for backward compatibility but not use them in the new code. I first need to create a set of representative cross-compilation tests.

This PR should not be merged if I do not manage to keep the binary compatibility.

@nicolasstucki nicolasstucki force-pushed the fix-inline-accessors branch 2 times, most recently from 6fbd4cf to 85318d8 Compare February 3, 2023 09:24
@nicolasstucki nicolasstucki added the needs-minor-release This PR cannot be merged until the next minor release label Feb 3, 2023
@nicolasstucki nicolasstucki force-pushed the fix-inline-accessors branch 3 times, most recently from d17088d to eb918a6 Compare February 7, 2023 09:28
Only inline accessors can add accessors to static members defined
outside their top-level class. These get the package owning the top-level
class from `hostForAccessorOf`.
If a class `C` needs inline accessors that would be added top-level
or if the accessor is to a static member, we place it in a new
invisible module `C$inline$accessors`.

If the accessor location in the new scheme is not the same as the
previous location, we also generate the old accessor for backward binary
compatibility but do not use it.

Fixes scala#13215
Fixes scala#15413
@nicolasstucki nicolasstucki marked this pull request as ready for review February 7, 2023 16:12
@nicolasstucki nicolasstucki requested review from odersky and sjrd February 8, 2023 08:22
@nicolasstucki nicolasstucki assigned sjrd and odersky and unassigned nicolasstucki Feb 8, 2023
@nicolasstucki nicolasstucki added this to the 3.4.0 milestone Feb 8, 2023
@nicolasstucki nicolasstucki added backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Feb 20, 2023
@nicolasstucki
Copy link
Contributor Author

Replaced with #16992

@nicolasstucki nicolasstucki removed this from the 3.4.0 milestone Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
3 participants