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

Change isStatic to isStaticOwner in hasLocalInstantiation #19803

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Feb 27, 2024

Fixes #19569
Related to #19569, but not a "fix". It actually allows eliminating outer references in even more cases.

@dwijnand
Copy link
Member

If you keep the printing you can demonstrate the change in the output, which I think is good evidence:

 List(public T1$C1())
-List(public T2$$anon$1$C2(T2$$anon$1))
-List(public T3$C3$1(T3$))
-List(public T4$$anon$2$C4(T4$$anon$2))
+List(public T2$$anon$1$C2())
+List(public T3$C3$1())
+List(public T4$$anon$2$C4())
 List(public T5$C5(T5))
 List(public T6$$anon$3$C6())
 List(public T7$C7$1())

@mbovel
Copy link
Member Author

mbovel commented Feb 27, 2024

Sounds good; I moved the test to generic-java-signature and added a check file (this should also solve the failing Scala.js tests).

@mbovel mbovel marked this pull request as ready for review February 28, 2024 06:30
@mbovel mbovel requested a review from dwijnand February 28, 2024 07:17
object T3 { def t3(): Unit = { class C3; test(classOf[C3]) } }
object T4 { def t4(): Unit = new AnyRef { class C4; test(classOf[C4]) } }

class T5 { class C5; test(classOf[C5]) } // outer ref currently not elided
Copy link
Member

Choose a reason for hiding this comment

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

I think "currently" isn't correct (and thus misleading). Classes that aren't defined in some kind of block are externally instantiable. So they are assumed to be public API, which means a change in the class's implementation can't break the API of that class's constructor.

Copy link
Member Author

@mbovel mbovel Mar 1, 2024

Choose a reason for hiding this comment

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

I see, this makes things clearer for me, thanks. I will clarify the comment.

But then, we should also not eliminate the outer reference in C1, should we? C1 is also publicly accessible and instantiable. Got it; no need for an outer pointer in this case; the object can be accessed directly from its path.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's assumed that T1$ is instantiated only once, by val T1, as opposed to multiple T5 instances.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comments updated.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM, except for 1 comment word 😄

Co-Authored-By: Dale Wijnand <[email protected]>
Co-Authored-By: noti0na1 <[email protected]>
Co-Authored-By: odersky <[email protected]>
@He-Pin
Copy link

He-Pin commented Mar 23, 2024

FYI:

val Tiker = "tick"
    class TickActor extends Actor {
      def receive:Receive = {
        case Tiker => // Do something
      }
    }

Will generate a constrocutor with String and

    class TickActor extends Actor {
      def receive:Receive = {
        case "tike"  => // Do something
      }
    }

will not.

@Kordyjan Kordyjan added this to the 3.4.2 milestone Mar 28, 2024
WojciechMazur added a commit that referenced this pull request Jul 3, 2024
…to LTS (#20974)

Backports #19803 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constructor not generated on 3.3.1
4 participants