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 augment clauses #4043

Closed
wants to merge 34 commits into from
Closed

Add augment clauses #4043

wants to merge 34 commits into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Feb 27, 2018

Introduce augment clauses. This PR is based on #4028.

The first new commit 945d2a8 introduces augment clauses with a syntax that makes it easy to augment generic classes, but less intuitive to augment specific instances of such classes.

The second new commit 5060aba introduces explicit type X binders in type patterns, which would make it possible to base the augment syntax on such patterns instead.

@nicolasstucki
Copy link
Contributor

The idempotency check is failing on tests/pos/augment.scala because the order of the methods is not the same in the byte code.

First run

public final class augments {
  public static <T> augments$TAugmentation_in_augments$$1<T> TAugmentation_in_augments$$1(T);
  public static <T> augments$TToHasEql_in_augments$$1<T> TToHasEql_in_augments$$1(T, augments$Eql<T>);
  public static <T> augments$RectangleToHasEql_in_augments$$1<T> RectangleToHasEql_in_augments$$1(augments$Rectangle<T>, augments$Eql<T>);
  public static augments$ArrayAugmentation_in_augments$$1 ArrayAugmentation_in_augments$$1(int[]);
  public static augments$ListAugmentation_in_augments$$2 ListAugmentation_in_augments$$2(scala.collection.immutable.List<java.lang.Object>);
  public static <T> augments$ListAugmentation_in_augments$$3<T> ListAugmentation_in_augments$$3(scala.collection.immutable.List<T>);
  public static <U> augments$ListAugmentation_in_augments$$1<U> ListAugmentation_in_augments$$1(scala.collection.immutable.List<scala.collection.immutable.List<U>>);
  public static <T> augments$RectangleAugmentation_in_augments$$1<T> RectangleAugmentation_in_augments$$1(augments$Rectangle<T>, augments$Eql<T>);
  public static augments$CircleToHasArea_in_augments$$1 CircleToHasArea_in_augments$$1(augments$Circle);
  public static augments$CircleAugmentation_in_augments$$1 CircleAugmentation_in_augments$$1(augments$Circle);
  public static <T> augments$Augmentation_in_augments$$1<T> Augmentation_in_augments$$1(scala.Tuple2<T, T>, augments$Eql<T>);
}

Second run

public final class augments {
  public static <T> augments$RectangleToHasEql_in_augments$$1<T> RectangleToHasEql_in_augments$$1(augments$Rectangle<T>, augments$Eql<T>);
  public static <T> augments$RectangleAugmentation_in_augments$$1<T> RectangleAugmentation_in_augments$$1(augments$Rectangle<T>, augments$Eql<T>);
  public static <T> augments$TToHasEql_in_augments$$1<T> TToHasEql_in_augments$$1(T, augments$Eql<T>);
  public static augments$CircleToHasArea_in_augments$$1 CircleToHasArea_in_augments$$1(augments$Circle);
  public static <U> augments$ListAugmentation_in_augments$$1<U> ListAugmentation_in_augments$$1(scala.collection.immutable.List<scala.collection.immutable.List<U>>);
  public static <T> augments$Augmentation_in_augments$$1<T> Augmentation_in_augments$$1(scala.Tuple2<T, T>, augments$Eql<T>);
  public static <T> augments$TAugmentation_in_augments$$1<T> TAugmentation_in_augments$$1(T);
  public static augments$ArrayAugmentation_in_augments$$1 ArrayAugmentation_in_augments$$1(int[]);
  public static augments$ListAugmentation_in_augments$$2 ListAugmentation_in_augments$$2(scala.collection.immutable.List<java.lang.Object>);
  public static augments$CircleAugmentation_in_augments$$1 CircleAugmentation_in_augments$$1(augments$Circle);
  public static <T> augments$ListAugmentation_in_augments$$3<T> ListAugmentation_in_augments$$3(scala.collection.immutable.List<T>);
}

@olafurpg
Copy link
Member

Exciting! One nice effect of implicit class is that it's possible to selectively import (or unimport) them.

object Enrichments {
  implicit class RichInt(x: Int) { def increment = x + 1 }
  implicit class UnwantedStringEnrichments(x: String) { def increment = x + "1" }
}
import Enrichments.RichInt
@ 1.increment
res0: Int = 2
@ "1".increment
cmd52.sc:1: value increment is not a member of String
val res52 = "1".increment
                ^
Compilation Failed

Would augment classes have something similar?

@odersky
Copy link
Contributor Author

odersky commented Feb 27, 2018

@olafurpg I was thinking of enabling optional naming by means of a pattern-style bind.

E.g

augment circleHasArea @ Circle extends HasArea {
   def area: Int = ....
}

So far it's just a thought... [EDIT: implemented by 3e10366]

@retronym
Copy link
Member

retronym commented Mar 1, 2018

Is it possible to express the equivalent of the following with an augment?

class Rich[T](val self: Either[T, T])  { def leftOrRight: T = ... }

I can't see how that would be expressed in the type pattern syntax without having to resort to an evidence parameter.

for `(x, y)`:

```scala
augment (type T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why parens instead of brackets?

augment [type T] {
  def ~ [U](that: U) = (this, that)
}
augment [type F[type A]](implicit functor: Functor[F]) {
  def map[B](f: A => B): F[B] = functor.map(this, 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.

It's just precedence - the type pattern syntax requires a sub-production of types. So to get a top-level type you need to parents. By contrast [...] would be something new and magic.

Inside an extension method, the name `this` stands for the receiver on which the
method is applied when it is invoked. E.g. in the application of `circle.circumference`,
the `this` in the body of `circumference` refers to `circle`. Unlike for regular methods,
an explicit `this` is mandatory to refer to members of the receiver. So the following
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike for regular methods, an explicit this is mandatory to refer to members of the receiver.

Why? I can imagine implementation-dependent reasons, but it's harder to tell why it makes sense for the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whilst I understand the reasoning, it is also yet another local rule.

def unwrap[T](x: WrappedResult[T]): T = x
}

def result[T](implicit er: WrappedResult[T]): T = WrappedResult.unwrap(er)
Copy link
Contributor

Choose a reason for hiding this comment

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

To be sure... this is a candidate for becoming def result[T]: WrappedResult[T] => T, right?

@Blaisorblade
Copy link
Contributor

Isn't it a bit premature to jump to the implementation before reviewing the design further? There are many interesting ideas, but I'm not sure all of them pull their weight.

Right now, this use of value classes:

package entity
class Person(val name: String) extends AnyVal

seems to become, after the replacement, the following:

package entity
object `package` {
  opaque type Person = String
  object Person {
    def apply(name: String): Person = name

    augment[Person] {
      def name = `this`
    }
  }
}

Or did I miss something from the proposals? I haven't found a similar example, and this seems pretty much a worst-case overhead — but it's the simplest use-case of value classes. (The example is based on one from Reddit FWIW).

That seems a significant amount of boilerplate, even after adding yet another way to handle extension methods to reduce the boilerplate.

Modifying value classes semantics instead

What about modifying the semantics of value classes to forbid user-defined toString and matching, and specifying they're sugar for the (translation of) the above? That seems to reduce migration overhead (outlaw those methods) and the amount of required new features.

Controlling visibility of construction/destruction

With opaque types, you write boilerplate to decide whether to allow construction or destruction to users. But value classes allow controlling this in the standard way!

class Person(private val name: String) extends AnyVal
class Person private(val name: String) extends AnyVal

If some people prefer to also have opaque types to have more control and can exhibit usecases, that's also an option, but it seems implicit value classes make augmentation sugar unnecessary.

A downside would be a more complicated translation, but it seems still feasible. And by our standards, we'd want to implement the translation in scalafix anyway, which is still inferior for users, and doesn't help people reading the generated boilerplate.

Also, the ones I propose seem the semantics I (and many) hoped from value classes in the beginning. I don't expect much of the implementation of value classes could be reused, but user code would be.

Maybe the JVM might allow reenabling them later without overhead, which is pretty easy to transition to. It's annoying for current users of toString, but any migration from current value classes will affect them and push them toward a Show typeclass or non-erasable wrappers.

Augmentation

Augmentation doesn't seem to improve much on implicit value classes. One downside with the latter is that they have names that can be used explicitly both for conversions and for types, but if it's desirable to make the conversion names optional (which I'm not sure about), it's easier to allow implicit class _(...) extends AnyVal and maybe implicit def _(...). As @OlivierBlanvillain pointed out, implicit defs are more powerful as they can produce existing types, instead of a fresh one, so it's more important to be able to hide them. But even for implicit class, the ability to hide them seems important.

If explicit usage of the type name is an anti-pattern, it might be easier to deprecate that aspect of implicit classes without adding a similar feature.

Copy link
Contributor

@Blaisorblade Blaisorblade left a comment

Choose a reason for hiding this comment

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

Here's also a few assorted comments on the design documents.

}
object Test {
import PostConditions._
val s = List(1, 2, 3).sum.ensuring(result == 6)
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so this example introduces a magic method result to avoid writing ensuring(_ == 6)? I see this is allowed, but this code seems to be pretty complicated relative to a pretty modest benefit — if this were the best use-case, I'd vote for rejecting the feature. I suppose there are better ones, but I'm not sure the feature is worth its weight there.

One downside with _ == 6 is that functions are harder to optimize than implicit functions, but shouldn't inline def ensuring(inline f: T => Boolean) work as well?

Copy link
Contributor Author

@odersky odersky Mar 7, 2018

Choose a reason for hiding this comment

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

I believe people who do contracts or something like Stainless or Liquid Haskell/Scala would disagree - this is essential for them, as result is essentially a keyword in these situations. An underscore does not cut it. Anyway, it's meant as a demonstration.

**Explanations**: We use an implicit function type `implicit WrappedResult[T] => Boolean`
as the type of the condition of `ensuring`. An argument condition to `ensuring` such as
`(result == 6)` will therefore have an implicit value of type `WrappedResult[T]` in scope
to pass along to the `result` method. `WrappedResult` is a fresh type, to make sure that we do not get unwanted implicits in scope (this is good practice in all cases where implicit parameters are involved). Since `WrappedResult` is an opaque type alias, its values need not be boxed, and since `ensuring` is added as an extension method, its argument does not need boxing either. Hence, the implementation of `ensuring` is as about as efficient as the best possible code one could write by hand:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you do have an implementation (even though that seems premature), shouldn't this be tested?
And wouldn't you need to sprinkle quite a few inline for this efficiency claim to be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be cool to add tests or benchmarks!

(elimTypeDefs.transform(tree), bindingsBuf.toList)
}

/** augment [<id> @] <type-pattern> <params> extends <parents> { <body>} }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still using brackets unlike the proposed docs which use parens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the brackets are EBNF syntax brackets here. Feel free to change to a better notation.


### Scope of Augment Clauses

Augment clauses can appear anywhere in a program; there is no need to co-define them with the types they augment. Extension methods are available whereever their defining augment clause is in scope. Augment clauses can be inherited or wildcard-imported like normal definitions. This is usually sufficient to control their visibility. If more control is desired, one can also attach a name to an augment clause, like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "inherited augment clauses" a useful pattern to recommend? Maybe yes, but it seems to warrant more thought.

This is usually sufficient to control their visibility.

That seems to be a bold claim to make without spelling out the evidence. Without a case study, there seems to be a risk that neither case gives a maintainable result for typical use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess some evidence is that a lot of other languages (Rust, C#, Kotlin) have extension methods or full augment clauses (in the case of Rust) without feeling the need for more precise scope control.

implicit class <id> <tparams> ($this: <T>) <params> extends <parents> { <body'> }

As before, `<body'>` results from `<body>` by replacing any occurrence of `this` with `$this`. However, all
parameters in <params> now stay on the class definition, instead of beging distributed to all members in `<body>`. This is necessary in general, since `<body>` might contain value definitions or other statements that cannot be
Copy link
Contributor

Choose a reason for hiding this comment

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

beging -> being

```scala
implicit class circleOps($this: Circle) extends HasArea {
def area = $this.radius * $this.radius * math.Pi
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires two more identifiers than the augmentation above, and that seems a small benefit relative to the additional complexity being introduced, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree. It's not just the identifiers.

 augment Circle extends HasArea { ... }

makes it super clear what we want to achieve. The implicit class syntax hides it to a large degree and is not intelligible at al to someone not used to it. There are 6 tokens before we even get to the thing that gets augmented, and then it's in a parameter list so visually disconnected from the thing it extends.


Conversely, it is conceivable (and desirable) to replace most usages of implicit classes and value classes by augmentations and [opaque types](../opaques.html). We plan to [drop](../dropped/implicit-value-classes.html)
these constructs in future versions of the language. Once that is achieved, the translations described
below can be simply composed with the existing translations of implicit and value classes into the core language. It is
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like one would want to translate augmentation without using the current value class translation, no? Viceversa, if we can simplify the translation of value classes by omitting features, why not just do that?

}
```

This augemntation makes `Circle` implement the `HasArea` trait. Specifically, it defines an implicit subclass of `HasArea`

Choose a reason for hiding this comment

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

augemntation -> augmentation

```scala
package shapeOps

augment circleOps @ Circle {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have top level augment clauses or they share the same restriction as implicit classes? If we can, we are missing a test case for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently these are not possible.

title: "Method Augmentations"
---

Method augmentations are a way to define _extension methods_. Here is a simple one:
Copy link
Member

Choose a reason for hiding this comment

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

What about "Method augmentation is" ?

@jvican
Copy link
Member

jvican commented Mar 5, 2018

My 2 cents: the augment language construct seems out of place and inconsistent with the whole language -- it cannot be explained in terms of classes and values as pretty much any Scala feature. I don't know if this is a problem or not, but I would like a more generic way of fitting extension methods in the language as we previously had with value classes -- something you can easily explain, and syntactic constructs you can easily predict from the existing Scala sytnax.

I think it may be better to introduce the concept of "augment companion" which would behave similar to how a companion does with the following exceptions:

  • lowercase names are encouraged.
  • users can define as many augment companions as they want.
  • you use augment object or some similar construct as the way to define it.

This is not a suggestion that rethinks how extension methods are proposed in this PR, I agree. But, at least, I find it easier to explain this way.

IMO, reusing existing concepts to represent extension methods is important to teach Scala developers Dotty better, as the gap between Scala and Dotty enlarges.

As an aside, I echo some of @Blaisorblade's comments, but I'm not as worried about the verbosity of the new approach to define value classes in Dotty compared to the previous one. That verbosity has more to do with opaque types (and the concept of opaque companions) than the introduction of augment, and I think it helps readability and gives a more principled way of organising Scala code.

@liufengyun
Copy link
Contributor

liufengyun commented Mar 5, 2018

I'm not sure if it is a good principle that a language construct that can be used in another module or play with scoping should always be named. It helps programmers to (1) communicate & reason about the design; (2) potentially better error messages, e.g., in the case of ambiguity.

Naming is also a justification of the design. If the language always requires a name for augment, the responsibility of abuse like the following is on programmer's part. If it happens locally, it's not a problem. But for it to be imported in other files, mandatory names seem to be better.

augment List[Int] {
  def foo: Int
}

augment List[Int] {
  def bar: Int
}

Can we restrict that nameless augment can only used locally, otherwise it has to be named?

@Blaisorblade
Copy link
Contributor

That verbosity has more to do with opaque types (and the concept of opaque companions) than the introduction of augment, and I think it helps readability and gives a more principled way of organising Scala code.

I agree that's mostly a comment on opaque types. It seemed augmentation was in part an answer to the verbosity of opaque types, but it does not reduce that significantly.

On opaque types, I like the semantics, but I'm torn on the syntax. I think I understand what you hint at, but I suspect people in fact dislike the semantics of value classes. But that discussion is indeed best done in another venue.


### Scope of Augment Clauses

Augment clauses can appear anywhere in a program; there is no need to co-define them with the types they augment. Extension methods are available whereever their defining augment clause is in scope. Augment clauses can be inherited or wildcard-imported like normal definitions. This is usually sufficient to control their visibility. If more control is desired, one can also attach a name to an augment clause, like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

s/whereever/wherever/

@sjrd
Copy link
Member

sjrd commented Mar 6, 2018

I have a bunch of high-level comments on this feature. TBH I think it hasn't been enough thought through.

The name

Why call this feature "augmentation" when it is well-known in the object-oriented world as extension methods? Bringing in the term "augment" is only asking for trouble in teaching and adoption of the language.

Optional names

Anonymous augmentations should be very rare, not the common case. There are at least two reasons for this, some of them have been mentioned already:

  • If they are not named, they cannot be selectively excluded from imports, nor selectively included in imports.
  • Anonymous members are a huge threat to binary compatibility.

These two reasons are related to one fundamental problem: this feature introduces anonymous members, a concept that has no precedent in the language. The only other anonymous thing that can come into scopes are implicit evidences for context bounds, but those are part of the lexical scope, never are they members.

Based on this, anonymous augmentations should only be valid in local-to-block scopes. But unlike implicit evidences, the use cases for local-to-block augmentations should be very rare.

This means that we shouldn't have anonymous augmentations at all: their existence is an additional tax on the specification and documentation that does not pull its weight. It also means that the syntax for named augmentations should be nicer, and I believe that the naming convention should be to start with an uppercase (like implicit classes).

Augmentations with extends

Those are misleading, as they suggest that one can make a class extend a trait or another class from the outside. However, this is not what happens, since there will be a wrapper when an instance is converted. This means that identity is broken, plus there is non-obvious boxing happening behind the scenes. The latter issue is known to have destroyed AnyVals in the minds of people. We must not repeat that mistake.

Augmentations with extends clauses should be a relatively rare use case compared to extension methods. They do not deserve special syntax. For the rare cases where the user wants this to happen, they can write their own implicit def with an associated class, which makes things very clear.

Therefore, I think they should not be supported.

Syntax

TBH, I think the proposed syntax is pretty ugly, especially for named augmentations (which, as argued before, should be the default/the only ones). Together with the problem of how the feature is named itself, I would suggest something based on the key word extension rather than augment, and which gives first-class aspect to named ones. Moreover, since augmentations with extends shouldn't exist, they don't need to be catered for. Maybe something like:

extension FooOps for Foo[String] {
  ...
}

This also provides a natural generalization to generic extensions that does not need to introduce a new type-matching construct:

extension FooOps[T] for Foo[T] {
}

This also works for fully generic extensions:

extension ToArrowAssoc[T] for T {
  def ->[U](that: U): (T, U) = (this, that)
}

We should also allow extensions to have implicit evidences that cannot be sugared into context bounds. Again, this syntax generalizes to that:

extension FooOps[T, U](implicit ev: Babar[T, U]) for Foo[T, U] {
}

(non-implicit term parameters must be disallowed, obviously)

Specification subtlety

The spec does not make it clear how augmentations relate to implicit conversions, and in particular does not specify the precedence of one over the other. In particular, what happens in this case:

class Foo {
  def bar: Int = 1
}

implicit def foo(x: String): Foo = new Foo

augment stringOps @ String {
  def bar: Int = 2
}

println("hello".bar) // does it print 1 or 2?

Also, in the body of an augmentation for a type C that redefines a method also existing in C, what does this.m refer to? Example:

augment stringOps @ String {
  def substring(x: Int): String = "hijacked"
  def foo(x: Int): String = this.substring(x)
}

Does this.substring(x) call String.substring(Int) or stringOps.substring(Int)?

Relatedly, I believe that having to explicitly write this is hardly justifiable to the end user. Either it should be allowed, or there should be strong justification for the restriction (that does not depend on implementation details).

@OlivierBlanvillain
Copy link
Contributor

There is one advantage to anonymity: it completely hides the underlying implicit conversion. I really dislike the fact that implicit classes can be abused as in the following:

implicit class PreName(s: String) {
  def toTermName = null
}

def foo(s: PreName) = s.toTermName

foo("test")

@sjrd
Copy link
Member

sjrd commented Mar 6, 2018

@OlivierBlanvillain Forcing augmentations to be named is not at odds with preventing your code snippet from compiling. To do that, we just make it so that there is no type named PreName in scope. There only needs to be the term named PreName which is in fact an implicit def.

@sjrd
Copy link
Member

sjrd commented Mar 6, 2018

@retronym's concern about how to translate

class Rich[T](val self: Either[T, T])  { def leftOrRight: T = ... }

is also supported out-of-the-box by the syntax I propose:

extension Rich[T] for Either[T, T] {
  def leftOrRight: T = ...
}

@Blaisorblade
Copy link
Contributor

On the method: I'd say such syntax fine-tunings need to make an even more compelling case and be held to higher standards, because they're highly visible to users while providing smaller benefits. That might include the SIP process/the contributors forum/...

On the merits:

There only needs to be the term named PreName which is in fact an implicit def.

So maybe it should be preName — this bugged me since we got implicit class. That's nice both for imports and for any possibly-needed explicit calls (since even Dotty can't promise complete inference). And explicit calls are another argument for having names.
Otherwise, it's possible that when Dotty can't infer some augmentation call in some user code, that user ends up having to modify the library defining it. (Sometimes the user might get away with copy-paste; not so if the augmentation uses inaccessible implementation details, hidden say through private[my.library.pkg]).

At that point, maybe, implicit class should be replaced with some other thing (augment/extension/some other keyword, with implicit or not); that seems sufficient to address concerns I see, and we get:

implicit extension preName(s: String) {
  def toTermName = new TermName(s)
}

instead of

implicit extension preName for String {
  def toTermName = new TermName(this)
}

The first is closer to what you have, the second lets you write this in exchange for a different syntax for arguments. Arguably, writing this is questionable because it behaves unlike this in several ways (you are not in the class scope, cannot call private methods, do not get a new identity, etc.), and because maybe you could wonder whether this refers to the wrapped object or to the wrapper.

On ambiguity, it'd be consistent to continue preventing hijacking, that's something good in Scala 2. To express that semantics, I suspect that

implicit extension stringOps(s: String) {
  def substring(x: Int): String = "hijacked"
  def foo(x: Int): String = s.substring(x)
}

is maybe (a bit!) clearer than

implicit extension stringOps for String {
  def substring(x: Int): String = "hijacked"
  def foo(x: Int): String = this.substring(x)
}

@sjrd
Copy link
Member

sjrd commented Mar 6, 2018

implicit extension preName(s: String) {

is pretty good too. We could also allow

implicit extension preName(this: String) {

(with this instead of s).

@Jasper-M
Copy link
Member

Jasper-M commented Mar 6, 2018

Minor nitpick. If you have

implicit extension preName(s: String)

then that would imply you can also have a non-implicit

extension preName(s: String)

I don't see the benefit of that.

Or if you can't have a non-implicit variant then implicit extension is just a pleonasm.

(Unless perhaps if extension would also implement the delegation/proxy pattern. Then non-implicit extensions might make sense.)

@Blaisorblade
Copy link
Contributor

@Jasper-M To clarify: extension alone wouldn't be allowed. The only reason for the pleonastic implicit is that currently all implicits use the implicit keyword, and it's maybe useful to keep it as a warning to readers.

@sjrd
Copy link
Member

sjrd commented Mar 6, 2018

In addition, that syntax allows to make extension a conditional keyword: it is considered as a keyword only if it immediately follows the keyword implicit, otherwise it is an identifier. This avoids breaking any existing code.

Exception to the above: def foo(implicit extension: Ext). If desired, we can go further and not make extension a keyword if it follows an implicit which itself follows a (. Not sure that's worth it, though.

@ShaneDelmore
Copy link
Contributor

Two quick thoughts.

  1. I prefer the syntax without the for, I know this isn’t worth much, but it just looks more like scala to me, as in closer to what I would guess I should write if I forgot the syntax.
  2. I also read augment and immediately though, hey, extensions, but on second look I can see the desire to name it something other than extensions due to scala’s existing usage of extends. When teaching the feature to new users we will either have to communicate “augment is similar to what you might know as extensions in other languages” or “no, extensions have no relation to extends” which could get pretty confusing.

@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2018

@srjd This is a trial balloon, intended to spark discussion. It seems that's what it has achieved!

Why call this feature "augmentation" when it is well-known in the object-oriented world as extension methods? Bringing in the term "augment" is only asking for trouble in teaching and adoption of the language.

I experimented with variations of extend/extension but felt that the overlap with extends was too great. If Scala used : for extends like C#, it would be possible, but the way things are I don't see it. If people want to propose a different name, please do!

Anonymous augmentations should be very rare, not the common case.

I disagree. Note that none of the other languages that have extension methods or full augmentations (like impl in Rust) have a way to name them.

[EDIT: I was not aware that C# does name extensions, however Kotlin and Rust don't]

If we feel that optional names are not the way to go forward, I would propose to drop them completely rather than making them mandatory.

If nevertheless more namespace control is desired one could always do this:

object circleOps {
   augment Circle {
     def circumference: Double = this.radius * math.Pi * 2
     def area: Double = this.radius * this.radius * math.Pi
   }
}

and then import circleOps._ instead of import circleOps.

Regarding binary compatibility, the naming scheme is chosen so that it is controllable, I think. It means that if you have in a compilation unit several augments of the same class for the same trait, or several extension method augments for the same class, you need to add the new ones at the end.

@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2018

One other thought: if augments can only do extension methods, we still need implicit classes or have to expose implicit conversions directly. I believe the use case that a class should be made to implement a trait after the class is defined is still very important. True, we get around this by systematically using type classes, at the price of more type parameters and a lot of implicit definitions. I would like to aim for a more direct way to do this.

We got used to implicit classes by now, but they are far from ideal syntax - they are are the same time too convoluted and too dense for a newcomer. Augment clauses make the intent much clearer.

@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2018

Another note on syntax: augment was chosen to mirror package. if you write

package Foo { ... }

You add something to a conceptually pre-existing package. Similarly

augment Foo { ... }

adds operations to Foo. I believe that should be it - the syntax should not be more complicated than that for the simple and common cases.

@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2018

rebased

@odersky
Copy link
Contributor Author

odersky commented Mar 8, 2018

Looking for syntax alternatives, here are two that I found most appealing so far. They both keep the structure of the previous syntax but change the keywords. So far, two typical cases are:

augment Circle { ... }
augment (type T: Eql) extends HasEql { ... }

Alternative 1: Replace augment with enhance:

enhance Circle { ... }
enhance (type T: Eql) extends HasEql { ... }

Alternative 2: Replace augment with extend and extends with a new keyword implements:

extend Circle { ... }
extend (type T: Eql) implements HasEql { ... }

What do people think? I believe enhance is better than augment because augment's connotations are too much in the quantitative sense. So far I like the last syntax best, even though it requires two new keywords. In particular it's nice that implements expresses "something like extends, but not quite the same thing".

@sjrd
Copy link
Member

sjrd commented Mar 8, 2018

About binary names: your suggestion addresses the objective issues I have. Subjectively I still prefer explicit names, but I won't fight for it.

About your alternative syntaxes: I'm still convinced that augmentations that extend some other class/trait shouldn't exist, so ... implements doesn't need to be there as far as I am concerned.

I don't understand the quantitative "issue" you have with augment: after all, an augmentation does add a measurable amount of new defs to a type. It is very quantitative.

@odersky
Copy link
Contributor Author

odersky commented Mar 8, 2018

About your alternative syntaxes: I'm still convinced that augmentations that extend some other class/trait shouldn't exist, so ... implements doesn't need to be there as far as I am concerned.

One possibility would be to allow only implementations of traits but not classes. That's sort of suggested by implements. I believe it would discourage most of the abuses, but keep the essential functionality, which IMO is worthwhile.

@sjrd
Copy link
Member

sjrd commented Mar 8, 2018

An augmentation that extends is not an abuse. It is a pitfall. See my earlier comment about the fact that they get boxed all the time in surprising ways.

@odersky
Copy link
Contributor Author

odersky commented Mar 8, 2018

I don't think boxing behavior needs to be surprising, since the rules are clear: An extension boxes if it contains an implements clause (assuming the last syntax). That makes it pretty obvious since all traits are reference types. I believe the previous pitfall was that a value class implementing a universal trait would still box when a trait method was called, but extends AnyVal somehow seemed to promise no boxing.

@OlivierBlanvillain
Copy link
Contributor

Maybe augment Cirle extends HasArea should simply add all the methods of HasArea to the Cirle augmentation without defining an implicit conversion. To illustrate what I mean, I propose to accept the following:

trait HasArea {
  def area: Int
  def radius = area / 2
}

augment Cirle extends HasArea {
  def area = ...
}

val c: Cirle = ..
println(c.radius)

But reject the following:

trait HasArea {
  def area: Int
}

augment Cirle extends HasArea {
  def area = ...
}

def foo(h: HasArea) = ..

val c: Cirle = ..
foo(c) // error: type mismatch;
       //  found   : Cirle
       //  required: HasArea

They are already two ways to achieve what this last snippet is trying to do: implicit conversions and type classes, why would we want to add a 3rd option? Also, shouldn't this be out of the scope of extension methods?

@odersky
Copy link
Contributor Author

odersky commented Mar 8, 2018

They are already two ways to achieve what this last snippet is trying to do: implicit conversions and type classes, why would we want to add a 3rd option?

Implicit conversions will be severely discouraged if not eliminated altogether. We should not propose them as a solution for anything. As to type classes, can you show what a solution would look like for Circle and HasArea? My current understanding is it would force you to do a global program tranformation by adding type parameters where there were none before. Or did you have something else in mind?

@sjrd
Copy link
Member

sjrd commented Mar 8, 2018

Deprecating implicit conversions, then allowing augmentations with extends, is ... kind of "kif-kif, bourricot", as we say in French (it's the same). The fact that it is limited to non-final/sealed target types (or even to non-sealed traits) is only reducing the attack surface, but not by any fundamental measure (only a quantitative one).

@odersky odersky mentioned this pull request Mar 8, 2018
@OlivierBlanvillain
Copy link
Contributor

OlivierBlanvillain commented Mar 8, 2018

This would be the type class way:

trait HasArea[T] {
  def area: Int
}

implicit val c = new HasArea[Cirle] {
  def area = ...
}

def foo[H: HasArea](h: H) = ..

val c: Cirle = ..
foo(c)

I don't think we gain anything if we discourage / remove implicit conversions and simply repeat the same mistakes with augment ... extends. Type classes are, in my opinion, the superior way to achieve ad-hoc polymorphism.

Like implicit conversions, augment ... extends falls short once we try to go beyond the simple example:

trait HasSize { def size }
trait HasArea { def area }
class Circle
augment HasArea extends HasSize { ... }
augment Circle extends HasArea { ... }

def foo(s: HasSize)
foo(new Circle) // Does not compile :-/
class Circle
trait HasArea { def area }
augment Circle extends HasArea { ... }

trait HasShape { def shape }
augment Circle extends HasShape { ... }

def foo(s: HasArea & HasShape) // How do I do that?
foo(new Circle)

@odersky
Copy link
Contributor Author

odersky commented Mar 8, 2018

Replaced by #4085

@odersky odersky closed this Mar 8, 2018
@odersky odersky mentioned this pull request Mar 14, 2018
@Sciss
Copy link
Contributor

Sciss commented Mar 15, 2018

Does this allow extension of objects, like (silly example)

implicit class VectorHasRandom(x: Vector.type) {
  def random(n: Int): Vector[Double] = Vector.fill(n)(math.random)
}

Vector.random(10)

That's possible with implicit classes now, and so augment class Vector versus augment object Vector would feel intuitive. Although, I guess, it could also be augment Vector.type...?

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.

None yet