Rename @alpha to @targetName #10149

odersky · 2020-11-02T18:47:51Z

... and make @TargetNAME work to avoid double definitions. Two definitions now match only if they have the same names, signatures and erased names. The erased name of a definition is the name given in a @targetName annotation, or else
the declared name, if no @targetName annotation is given.

Fixes #7942

soronpo · 2020-11-04T13:27:36Z

Does this change have any effect over the possibility solving #7942 ?

odersky · 2020-11-04T15:34:08Z

@soronpo Yes, this fixes #7942.

soronpo · 2020-11-05T03:47:28Z

docs/docs/reference/other-new-features/targetName.md

+  |      Name clash between defined and inherited member:
+  |      def f(): Int in class A at line 3 and
+  |      def g(): Int in class B at line 5
+  |      have the same name and type after erasure.


Regarding the error message when no @targetName is applied, maybe it's worth proposing the user the option of using @targetName to resolve the error within the error message itself.

soronpo · 2020-11-05T13:32:52Z

BTW, is there a purpose for DummyImplicit now, or can it be deprecated? AFAIK, it was only used to define unique signatures.

odersky · 2020-11-05T15:30:32Z

I think DummyImplicit could be deprecated. But it lives in 2.13, so for now it's untouchable :-)

liufengyun

LGTM, with some minor comments.

The following code now compiles:

import scala.annotation.targetName

class B {
  final def foo(): Int = ???
  def bar(): Int = ???
}

class C extends B {
  @targetName("fooB") def foo(): Int = ???
  @targetName("barB") def bar(): Int = ???
}

Instead, I think the compiler should report two errors above. The reason is that @targetName is intended to overcome the limitations of target platforms, rather than change the (static) semantics of the surface language.

The following code currently compiles, it should be rejected:

class C {
  @targetName("foo B") def foo(): Int = ???
}

The following code currently compiles, should be rejected:

class C(x: String) {
  @targetName(x) def foo(): Int = ???
}

/cc @bishabosha Could you please comment about Scala 2 forward interoperability?

The following comment is intended for open discussions in the future, not for this PR.

explicit annotation VS synthesized annotation

The advantage of explicit annotation is obvious: better debuggability and Java interoperability.

On the other hand, when programmers don't care about Java interoperability & debuggability (e.g. during prototyping), can the compiler automatically synthesize target names to overcome the limitations of the target platform?

There are already many places where the compiler performs synthesis to overcome platform limitations, is it possible to hide the implementation detail for targetName by default, and allows users to optionally use @targetName when they care about debuggability and Java Interop?

docs/docs/reference/other-new-features/targetName.md

compiler/src/dotty/tools/dotc/typer/Typer.scala

sjrd · 2020-11-07T17:48:20Z

The following code currently compiles, it should be rejected:
class C {
  @targetName("foo B") def foo(): Int = ???
}

I don't think so. I believe the only characters that should be rejected are . ; / [ <, as those are the characters that are not valid in a JVM method name. For classes we should also allow <.

We should forbid unpaired surrogate characters as well.

soronpo · 2020-11-07T21:33:07Z

The following code currently compiles, it should be rejected:
class C {
  @targetName("foo B") def foo(): Int = ???
}
I don't think so. I believe the only characters that should be rejected are . ; / [ <, as those are the characters that are not valid in a JVM method name. For classes we should also allow <.

We should forbid unpaired surrogate characters as well.

But Scala 2 allows def `.;/[<` : Unit = {} . What happens at the backend in this case? Does Scala rename all of these characters?

smarter · 2020-11-07T21:35:12Z

These characters get encoded: https://github.com/scala/scala/blob/0a1942c82f18f11cf6d39d0a1e300e9995dcdbfc/src/library/scala/reflect/NameTransformer.scala#L45-L62

liufengyun · 2020-11-08T07:37:31Z

The following code currently compiles, it should be rejected:
class C {
  @targetName("foo B") def foo(): Int = ???
}
I don't think so. I believe the only characters that should be rejected are . ; / [ <, as those are the characters that are not valid in a JVM method name. For classes we should also allow <.

We should forbid unpaired surrogate characters as well.

Good point, as Scala already allows such names with backquotes, for consistency they need to be supported (However, for better platform portability, it seems better for language design decisions to be independent of a particular platform).

sjrd · 2020-11-08T08:18:55Z

(However, for better platform portability, it seems better for language design decisions to be independent of a particular platform).

In practice, the other platforms already have to handle what the JVM can handle.

odersky · 2020-11-08T11:01:33Z

I don't think so. I believe the only characters that should be rejected are . ; / [ <, as those are the characters that are not valid in a JVM method name. For classes we should also allow <.

We should forbid unpaired surrogate characters as well.

The backend encodes names so that illegal method names are avoided. For instance,

def `..` = 1

will end up as

public int $u002E$u002E();

@targetName should behave exactly like backticks. Either they both admit all idents and avoid bad ones by encodings, or both should reject illegal names outright. So this is a separate issue.

odersky · 2020-11-08T11:31:04Z

@liufengyun

Instead, I think the compiler should report two errors above. The reason is that @TargetNAME is intended to overcome the limitations of target platforms, rather than change the (static) semantics of the surface language.

That's quite a fundamental question we need to answer here. Should @TargetNAME be allowed to refine the overriding behavior? Right now the answer is yes. If we change that it could get tricky and also expensive. Here's a problematic example:

trait A:
  def f: Any
trait B:
  @targetName("ff") def f: Int
abstract class C extends A, B

That's currently legal. If it should be illegal, this means that that no class could ever extend both A and B. Even if we tried to add implementations to class C like this:

class C extends A, B:
  override def f: Any = ""
  @targetName("ff") override def f: Int = 1

it still would not work since each of the two definitions of f in C "overrides" one other instance with a different target name.

So this would be a pretty harsh restriction, and it's particular annoying since it arises from global behavior and cannot be controlled by local knowledge alone.

The other objection is that even checking a situation like this is expensive. Enumerating overriding pairs is quadratic in the size of the program. It's a significant part of total compile-time when compiling things like stdlib. Here we would make it twice as expensive.

On the other hand, what would we really gain from a restriction?

odersky · 2020-11-08T11:34:51Z

The following code currently compiles, should be rejected:

class C(x: String) {
  @targetName(x) def foo(): Int = ???
}

Is there a way to do this by annotating the definition of the targetName class in some way? I.e. can we force annotations to take only String literals as parameters?

liufengyun · 2020-11-08T13:01:41Z

@liufengyun

Instead, I think the compiler should report two errors above. The reason is that @TargetNAME is intended to overcome the limitations of target platforms, rather than change the (static) semantics of the surface language.

That's quite a fundamental question we need to answer here. Should @TargetNAME be allowed to refine the overriding behavior? Right now the answer is yes. If we change that it could get tricky and also expensive. Here's a problematic example:
trait A:
  def f: Any
trait B:
  @targetName("ff") def f: Int
abstract class C extends A, B
That's currently legal. If it should be illegal, this means that that no class could ever extend both A and B. Even if we tried to add implementations to class C like this:
class C extends A, B:
  override def f: Any = ""
  @targetName("ff") override def f: Int = 1
it still would not work since each of the two definitions of f in C "overrides" one other instance with a different target name.

To clarify I think the following example will work:

trait A {
  def f: Any
}

trait B {
  @targetName("ff")
  def f: Int
}

abstract class C extends A with B {
  def f: Any = ""
  @targetName("ff")
  def f: Int = 3
}

The reason is that overriding check takes the whole type into consideration, while conflicit check only takes signature and target name into consideration.

So this would be a pretty harsh restriction, and it's particular annoying since it arises from global behavior and cannot be controlled by local knowledge alone.

The other objection is that even checking a situation like this is expensive. Enumerating overriding pairs is quadratic in the size of the program. It's a significant part of total compile-time when compiling things like stdlib. Here we would make it twice as expensive.

What I propose here is the following:

In overriding check, use original names and in addition make sure that overridding pairs have the same target names
In conflict member check, use target names.

With the scheme above, will performance be less a concern?

On the other hand, what would we really gain from a restriction?

I think it is safer to guard against abuse of language features:

class A {
  final def f(): Int = ???
}

class B extends A {
  @targetName("ff")
  def f(): Int = ???
}

odersky · 2020-11-08T14:19:08Z

The reason is that overriding check takes the whole type into consideration, while conflicit check only takes signature and target name into consideration.

No that does not work. ()Int is a legal override of ()Any.

liufengyun · 2020-11-08T15:08:55Z

The reason is that overriding check takes the whole type into consideration, while conflicit check only takes signature and target name into consideration.

No that does not work. ()Int is a legal override of ()Any.

Yes, I missed that Int <: Any. The essential question seems to be

Only use the feature to remove platform restrictions, or
Also use it to make the surface language more expressive

The use cases intended by the first is clear, while it is unclear for the 2nd. Another worry about the 2nd is that it may create inconsistencies. For example, can we override the target name directly?

trait A {
  @targetName("ff")
  def f(): Int
}

class B extends A {
  def ff(): Int = ???
}

In the current PR, the code above is rejected, which makes reasoning about overriding more complex.

odersky · 2020-11-08T15:27:26Z

In the current PR, the code above is rejected, which makes reasoning about overriding more complex.

Here's what's stated in targetName.md:

@targetName annotations are significant for matching two method definitions to decide whether they conflict or override each other. Two method definitions match if they have the same name, signature, and erased name. Here,

The signature of a definition consists of the names of the erased types of all (value-) parameters and the method's result type.
The erased name of a method definition is its target name if a @targetName
annotation is given and its defined name otherwise.

So, both regular names and target names need to match, otherwise it's not an override. After erasure, it's only the targetname that matters, but it is verified that this change does not introduce any new overrides.

bishabosha · 2020-11-09T15:56:45Z

The changes to Tasty should be fine in this case for Scala 2 forward compat, it makes it easier to work with than just the alpha annotation

This will streamline some of the later matching code.

Alternatives that have the same signature but different targetNames are considered to be separate.

So far, annotations of symbols printed as just the class name; also include their arguments. But avoid this for Body annotations, since they just repeat the possibly large body of an inline function.

@TargetNAME

In case of a double definition error that's due to an erasure clash, add a hint that one can resolve it with a @TargetNAME annotation

@TargetNAME

Don't issue the hint of one of the conflicting definitions already carries a @TargetNAME.

Co-authored-by: Fengyun Liu <liu@fengy.me>

odersky · 2020-11-10T12:18:58Z

test performance please

dottybot · 2020-11-10T12:19:28Z

performance test scheduled: 4 job(s) in queue, 1 running.

dottybot · 2020-11-10T15:04:48Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/10149/ to see the changes.

Benchmarks is based on merging with master (aaec84d)

odersky · 2020-11-10T15:39:20Z

test performance please

dottybot · 2020-11-10T15:40:24Z

performance test scheduled: 8 job(s) in queue, 1 running.

odersky · 2020-11-10T16:34:52Z

We now restrict the power of @TargetNAME where it deviates from normal semantics. The new rules are as follows:

Two members can override each other if their names and signatures are the same,
and they either have the same erased names or the same types.
If two members override, then both their erased names and their types must be the same.

So this means that targetNames can only avoid breakage due to erasure clashes. They cannot introduce new distinctions on their own.

I also found a way to make this efficient. The OverridingPairs cursor in RefChecks implements exactly the behavior described above. Then we check for each returned pair in checkOverride that both erased names and types match. So we still need only a single pass to do this.

@liufengyun Can you review the latest commits implementing this?

liufengyun

LGTM

dottybot · 2020-11-10T17:40:28Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/10149/ to see the changes.

Benchmarks is based on merging with master (0700573)

odersky force-pushed the fix-alpha branch 2 times, most recently from ebe5828 to 40f7b3d Compare November 4, 2020 12:42

odersky marked this pull request as ready for review November 4, 2020 12:43

soronpo reviewed Nov 5, 2020

View reviewed changes

liufengyun approved these changes Nov 7, 2020

View reviewed changes

docs/docs/reference/other-new-features/targetName.md Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved

odersky force-pushed the fix-alpha branch from 1a163a0 to 553bc56 Compare November 8, 2020 11:38

odersky added 7 commits November 10, 2020 08:28

Rename @Alpha to @TargetNAME

118f419

Define NoSymbol.erasedName to be EmptyTermName

6eaa10e

This will streamline some of the later matching code.

Allow to disambiguate overloads with @TargetNAME

8f16a33

Alternatives that have the same signature but different targetNames are considered to be separate.

Handle superAccessors in the presence of @TargetNAME

b3d0c47

Print arguments of annotations

419cd87

So far, annotations of symbols printed as just the class name; also include their arguments. But avoid this for Body annotations, since they just repeat the possibly large body of an inline function.

Handle retained inline bodies for methods with @TargetNAME annotations

b7b1e1b

Update reference pages

87bce7b

odersky and others added 11 commits November 10, 2020 08:28

Update test to show that scala#7942 is fixed

270f661

Fix handling of RetainedInline when compiling from tasty

3b0f98c

Update features classification doc page

1195bd1

Factor out common behavior for target name annotations

fca3345

Rename erasedName -> targetName

54337a8

Streamline targetName matching tests

b27633b

Add hint to error message

ce53d89

In case of a double definition error that's due to an erasure clash, add a hint that one can resolve it with a @TargetNAME annotation

Refine @TargetNAME hinting

9deb808

Don't issue the hint of one of the conflicting definitions already carries a @TargetNAME.

Apply suggestions from code review

aedf313

Co-authored-by: Fengyun Liu <liu@fengy.me>

Avoid @ in sidebar titles

039f42f

Enforce matching targetNames when overriding

6310ec2

odersky force-pushed the fix-alpha branch from 3edfa64 to 6310ec2 Compare November 10, 2020 10:59

Add clause to docs

d4fca54

Require string literals as target name arguments

773023b

odersky force-pushed the fix-alpha branch from 0fa0548 to 773023b Compare November 10, 2020 15:09

liufengyun approved these changes Nov 10, 2020

View reviewed changes

liufengyun merged commit 4411811 into scala:master Nov 10, 2020

liufengyun deleted the fix-alpha branch November 10, 2020 17:12

Kordyjan added this to the 3.0.0 milestone Aug 2, 2023

Rename @alpha to @targetName #10149

Rename @alpha to @targetName #10149

Uh oh!

Conversation

odersky commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soronpo commented Nov 4, 2020

Uh oh!

odersky commented Nov 4, 2020

Uh oh!

soronpo Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soronpo commented Nov 5, 2020

Uh oh!

odersky commented Nov 5, 2020

Uh oh!

liufengyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

explicit annotation VS synthesized annotation

Uh oh!

Uh oh!

Uh oh!

sjrd commented Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soronpo commented Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Nov 7, 2020

Uh oh!

liufengyun commented Nov 8, 2020

Uh oh!

sjrd commented Nov 8, 2020

Uh oh!

odersky commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Nov 8, 2020

Uh oh!

odersky commented Nov 8, 2020

Uh oh!

liufengyun commented Nov 8, 2020

Uh oh!

odersky commented Nov 8, 2020

Uh oh!

liufengyun commented Nov 8, 2020

Uh oh!

odersky commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bishabosha commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Nov 10, 2020

Uh oh!

dottybot commented Nov 10, 2020

Uh oh!

dottybot commented Nov 10, 2020

Uh oh!

odersky commented Nov 10, 2020

Uh oh!

dottybot commented Nov 10, 2020

Uh oh!

odersky commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

dottybot commented Nov 10, 2020

Uh oh!

Uh oh!

odersky commented Nov 2, 2020 •

edited

Loading

soronpo Nov 5, 2020 •

edited

Loading

liufengyun left a comment •

edited

Loading

sjrd commented Nov 7, 2020 •

edited

Loading

soronpo commented Nov 7, 2020 •

edited

Loading

odersky commented Nov 8, 2020 •

edited

Loading

odersky commented Nov 8, 2020 •

edited

Loading

bishabosha commented Nov 9, 2020 •

edited

Loading

odersky commented Nov 10, 2020 •

edited

Loading