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

Trial balloon: Make default getters final #16505

Closed
wants to merge 1 commit into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Dec 12, 2022

I want to see what the fallout is when we make default getters final, which is necessary if we want to make them inlineable.

So far I have seen ~20 occurrences in the compiler codebase itself and a handful more in the tests where an overriding function has exactly the same default argument as the overridden function, so the repeated default argument was redundant.

I have seen no occurrences in this codebase where the overriding function had a meaningful different default argument, except for tests that were specifically designed to test that feature.

I want to see what the fallout is when we make default getters final, which
is necessary if we want to make them inlineable.

So far I have seen ~20 occurrences in the compiler codebase itself and a handful more
in the tests where an overriding function has exactly the same default argument as the
overridden function, so the repeated default argument was redundant.

I have seen no occurrences in this codebase where the overriding function had a
meaningful different default argument, except for tests that were specifically designed
to test that feature.
@sjrd
Copy link
Member

sjrd commented Dec 12, 2022

As I mentioned off-line a while ago, this is going to be problematic for Scala.js. A typical use case is the following:

@js.native @JSGlobal("NativeParent")
class NativeParent extends js.Object {
  def foo(x: Int, y: Int = js.native): Int = js.native
}

class NonNativeChild extends NativeParent {
  override def foo(x: Int, y: Int = 5): Int = x + y
}

Since default values are handled at callee site for JS classes, the NonNativeChild must provide a new default value for y. It cannot "inherit" the default value from its parent, since it would only be in the body of NativeParent.foo.

We test this in NonNativeJSTypeTest.overrideMethodWithDefaultValuesFromNativeJSClass (among many other tests for default values).

Note that for this to work, we don't actually need late binding of the default method calls. They are called from within the generated body of foo, not from the call site. So enforcing early binding without preventing overrides would be OK for this to work.

@odersky
Copy link
Contributor Author

odersky commented Dec 12, 2022

Note that for this to work, we don't actually need late binding of the default method calls. They are called from within the generated body of foo, not from the call site. So enforcing early binding without preventing overrides would be OK for this to work.

Of course in general, going from late to early binding is a behavior change, so we have to be very careful here. Can we find criteria that tell us that the change is OK and we should still accept the override?

Besides ScalaJS I was thinking to allow overrides if the two default argument expressions are the same since that seems to be a common case. Maybe warn but don't reject.

@sjrd
Copy link
Member

sjrd commented Dec 12, 2022

Can we find criteria that tell us that the change is OK and we should still accept the override?

The criteria for the Scala.js-specific use case is whether the enclosing class extends js.Any. In other words, whether ctx.settings.scalajs.value && ownerClass.isJSType is true.

@odersky odersky closed this Dec 14, 2022
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.

2 participants