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

Add @experimental annotation #12102

Merged
merged 13 commits into from
May 10, 2021

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Apr 15, 2021

The @exerimental annotation marks definitions as experimental feature.
These can be used in the same situations where languange.experimental can be used.

  • A class is experimental if
    • It is annotated with @experimental
    • It is a nested class of an experimental class. Annotation @experimental is inferred.
    • It extends an experimental class. An error is emitted if it does not have @experimental.
  • A member definition is experimental if
    • It is annotated with @experimental
    • All overridden definitions are experimental
    • Its owner is an experimental class
  • class experimental is experimental

@nicolasstucki nicolasstucki force-pushed the add-experimental-annotation branch 6 times, most recently from f5b4430 to 290f36d Compare April 16, 2021 10:46
@nicolasstucki
Copy link
Contributor Author

@sjrd are there some important test cases that I may have missed?

@smarter for the comments in def isExperimental should we infer @exerimental of member definitions or just look each time. I cannot find a precedent where we add an annotation like this while typing.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

@sjrd are there some important test cases that I may have missed?

Concrete type alias (danger of being dealiased) and opaque type alias are two good candidates. Otherwise it seems pretty exhaustive. :)

@smarter smarter added this to the 3.0.0-RC3 milestone Apr 16, 2021
@smarter smarter added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Apr 16, 2021
compiler/src/dotty/tools/dotc/config/Feature.scala Outdated Show resolved Hide resolved
def isExperimental(using Context): Boolean =
(self eq defn.ExperimentalAnnot)
|| self.hasAnnotation(defn.ExperimentalAnnot)
|| self.allOverriddenSymbols.nonEmpty && self.allOverriddenSymbols.forall(_.hasAnnotation(defn.ExperimentalAnnot)) // TODO infer @experimental?
Copy link
Member

Choose a reason for hiding this comment

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

I would say a method is experimental if at least one method it overrides is experimental:

Suggested change
|| self.allOverriddenSymbols.nonEmpty && self.allOverriddenSymbols.forall(_.hasAnnotation(defn.ExperimentalAnnot)) // TODO infer @experimental?
|| self.allOverriddenSymbols.exists(_.hasAnnotation(defn.ExperimentalAnnot)) // TODO infer @experimental?

but I think more properly when we do override checks in RefChecks, we should disallow overriding an experimental method from a non-experimental method, same with subclassing an experimental class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For methods, I am not sure what we should do with the following case

trait A:
  @experimental def f: Int
trait B:
  def f: Int
trait C extends A, B:
  def f: Int = 3

Is C.f experimental? It implements an experimental API but at the same time it implements a stable API. If we look at it as implementing B.f it can be considered as non-experimental.

Copy link
Member

Choose a reason for hiding this comment

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

It implements both and if A.f is removed that might have binary compatibilty implications on C.f and its users, so yes.

Copy link
Member

@sjrd sjrd Apr 16, 2021

Choose a reason for hiding this comment

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

Applying the LSP, the only thing that should be forbidden is when an experimental method implements/overrides a non-experimental one, for example:

trait A:
  def f: Int
  def g: Int = 3
trait B extends A:
  @experimental def f: Int = 4 // error
  @experimental override def g: Int = 5 // error

because if that were allowed, one could call B.f or B.g through an A without being detected.

It's perfectly fine to implement or override an experimental method with a non-experimental one.

Copy link
Member

Choose a reason for hiding this comment

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

It's perfectly fine to implement or override an experimental method with a non-experimental one.

I'm not convinced, it's at least not source-compatible since if that method gets removed, your usage of override is now an error. I would err on the side of caution and disallow both directions.

Copy link
Member

Choose a reason for hiding this comment

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

It implements both and if A.f is removed that might have binary compatibilty implications on C.f and its users, so yes.

No, it can't. Users of C.f will always produce bytecode that call C.f itself, never A.f or any of the bridges generated in C to adapt it to A.f.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I think more properly when we do override checks in RefChecks, we should disallow overriding an experimental method from a non-experimental method

Do you mean that we need to annotate each experimental method with @experimental? That would imply that all members of an experimental class must be individually annotated.

Copy link
Member

Choose a reason for hiding this comment

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

If the class itself is experimental then when you subclass it, the subclass should be marked experimental, but no need to mark the individual methods as experimental

Copy link
Member

Choose a reason for hiding this comment

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

It's perfectly fine to implement or override an experimental method with a non-experimental one.

I'm not convinced, it's at least not source-compatible since if that method gets removed, your usage of override is now an error. I would err on the side of caution and disallow both directions.

Any change of public/protected API, even experimental, is always going to be source incompatible. Removing an experimental top-level class could change how some name resolution behaves, even if it did not actually resolve to that class before. You can't protect people from theoretical source incompatibilities. Don't try.

compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/util/Experimental.scala Outdated Show resolved Hide resolved
@smarter smarter assigned nicolasstucki and unassigned smarter Apr 16, 2021
@nicolasstucki nicolasstucki force-pushed the add-experimental-annotation branch 5 times, most recently from 6a12e15 to c23db64 Compare April 16, 2021 14:05
compiler/src/dotty/tools/dotc/config/Feature.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/config/Feature.scala Outdated Show resolved Hide resolved
object FromDigits {

/** A subclass of `FromDigits` that also allows to convert whole number literals
* with a radix other than 10
*/
@experimental
Copy link
Member

Choose a reason for hiding this comment

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

Is it necesary to mark these definitions as experimental since their enclosing object is already experimental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is by the inheritance check. We could add an exception, but then we would need the annotation inference again or make the isExperimental more computationally expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best solution I found is to infer all @experimental on classes nested in an @experimental definition and then require the annotation on any class that extends an experimental class. This implies that if we have a class that is both nested in experimental class and extends an experimental class it will infer the annotation. This also implies that we only need to look at the direct parents to do the checks.

@odersky
Copy link
Contributor

odersky commented Apr 17, 2021

Can this be added in 3.0.1? I am wary of any additional restrictions we impose so late in the game. Anything that makes some programs not compile should be deferred.

@smarter
Copy link
Member

smarter commented Apr 17, 2021

We could put the annotation in the library now but only give it meaning in 3.0.1, this does mean we can't rely on it for FromDigits but that's not a big deal I think.

The `@exerimental` annotation marks definitions as _experimental_ feature.
These can be used in the same situattions where `languange.experimental` can be used.

* A class is experimental if
  * It is annotated with `@experimental`
  * It is a nested class of an experimental class. Annotation `@experimental` is inferred.
  * It extends an experimental class. An error is emitted if it does not have @experimental.
* A member definition is experimental if
  * It is annotated with `@experimental`
  * Its a member of an experimental class
* `class experimental` is experimental

private def annotateExperimental(sym: Symbol)(using Context): Unit =
if sym.is(Enum) && sym.hasAnnotation(defn.ExperimentalAnnot) then
// Add @experimental annotation to enum class definitions
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed if we also have checkExperimentalInheritance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed anymore. Removed the code. Though now there is similar looking logic for case class modules.

@@ -0,0 +1,31 @@
import scala.annotation.experimental

@experimental // error
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we don't have any test for an @experimental case class, I think we need to make sure that the generated companion is also marked @experimental (or else just always check both the class and the companion since having one experimental but not the other isn't very useful).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the annotation in annotateExperimental.

@nicolasstucki nicolasstucki merged commit bdab077 into scala:master May 10, 2021
@nicolasstucki nicolasstucki deleted the add-experimental-annotation branch May 10, 2021 12:49
@Kordyjan Kordyjan added this to the 3.0.1 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants