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 extension method syntax #7557

Merged
merged 7 commits into from
Nov 18, 2019
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Nov 15, 2019

It's now

given listOps: extension [T](xs: List[T]) { ... }

given extension (s: String) { ... }

instead of

given listOps: [T](xs: List[T]) { ... }

given (s: String) { ... }

I hope that makes it easier to read and to search for.

@sjrd
Copy link
Member

sjrd commented Nov 15, 2019

Yes!!!

@smarter
Copy link
Member

smarter commented Nov 15, 2019

given listOps: extension [T](xs: List[T])

What's the rationale for preferring this over something like:

given extension listOps[T](xs: List[T])

@neko-kai
Copy link
Contributor

What's the rationale for preferring a given prefix at all? If we go for intent over mechanism, then the intent here is to declare extension methods - the fact that an implicit object is involved in this somehow is completely secondary and mechanism-oriented! So, why not:

extension [T](xs: List[T])

@bishabosha
Copy link
Member

Well unless you want an import foo.extension then you still need to import the object by name or with import foo.given - so if given is not on the definition its less obvious

@neko-kai
Copy link
Contributor

@bishabosha Why not import individual extension methods by name? IMHO the object is purely a strange implementation detail, why would a top-level extension method not be imported with .given, but a grouped extension method would be? e.g. what's the difference between following two declarations that necessitates the object mechanism to become relevant?

def (i: Int) inc: Int = i + 1

vs.

extension (i: Int) {
  def inc: Int = i + 1
}

@odersky
Copy link
Contributor Author

odersky commented Nov 15, 2019

What's the rationale for preferring this over something like:

given extension listOps[T](xs: List[T])

Regularity. If a given has a name id, it always looks like id:. This also makes it clearer that the parameters go on the extension methods, not on the wrapper.

@odersky
Copy link
Contributor Author

odersky commented Nov 15, 2019

@neko-kai It's too late to claim extension as a hard keyword, and it cannot be a soft keyword when used to start a definition -- too much scope for ambiguity.

Besides, these things are givens, they are available in the implicit scope of companion classes and have to be imported as using import given. So it's better to make that clear in the syntax, I believe.

@liufengyun
Copy link
Contributor

The grouped extension method syntax always creates a wrapper object, which is not composable with existing Scala langauge features.

(1) It is impossible to declare a group of abstract extension methods in a trait and later implement them in a class.

(2) It's impossible to bind grouped extension methods to a known object/class/trait

(3) It's impossible to split grouped extension methods to a trait, and mixin the methods in a known class or object.

In contrast, if we allow an object/trait/class may have 0-n extension clauses (each is a grouped extension), we may avoid the problems above.

@odersky
Copy link
Contributor Author

odersky commented Nov 15, 2019

@liufengyun The object is actually needed for implicit scope semantics. E.g.

class List[+T] with ...

object List with
  given extension [T](xss: List[List[T]]) with
    def flatten: List[T] = ...

To make flatten available without import it has to be put where it is. The other worry about "wrapper-less" grouped extension methods is that it would make all collectively defined extension methods available under their own name in the enclosing scope. I believe it's overall more manageable to default to grouping them with a wrapper that can be imported selectively. But others might prefer the alternative.

@liufengyun
Copy link
Contributor

liufengyun commented Nov 15, 2019

To abuse the keyword extends to support extension clauses, the code seems can be written as follows:

object List extends [T](xss: List[List[T]]) with
   def flatten: List[T] = ...

The other worry about "wrapper-less" grouped extension methods is that it would make all collectively defined extension methods available under their own name in the enclosing scope. I believe it's overall more manageable to default to grouping them with a wrapper that can be imported selectively.

Depending on whether extends or extension is used (or both), it seems this special case can be subsumed with extension clause:

given extension (x: String) {
   ...
}

given StringOps extends (x: String) {
   ...
}

It's just an anonymous/named given object with 0 templates and 1 extension clause.

@soronpo
Copy link
Contributor

soronpo commented Nov 16, 2019

@neko-kai It's too late to claim extension as a hard keyword, and it cannot be a soft keyword when used to start a definition -- too much scope for ambiguity.

I'm sorry, but "too late" in what time-context? FWIW, I proposed using extension back when given was introduced. #7151 (comment)

@odersky
Copy link
Contributor Author

odersky commented Nov 16, 2019

I'm sorry, but "too late" in what time-context? FWIW, I proposed using extension back when given was introduced. #7151 (comment)

About 10 years too late 😉

@neko-kai
Copy link
Contributor

neko-kai commented Nov 16, 2019

@odersky

@liufengyun The object is actually needed for implicit scope semantics.

We could make individual extension methods also obey implicit scope. Why not?

The other worry about "wrapper-less" grouped extension methods is that it would make all collectively defined extension methods available under their own name in the enclosing scope. I believe it's overall more manageable to default to grouping them with a wrapper that can be imported selectively. But others might prefer the alternative.

I think that's not a big issue, because due to overloading the names will be available most of the time anyway - it's also extremely intuitive for the user to be able to import methods from a group definition by their name AND easy for the IDE to find these methods and perform auto-import, for an anonymous given extension object how would an IDE even be able to import it selectively, if it's not referable?

Comparing the wrapper objects for specificity is not enough; we should also
look at the resolved methods themselves.
Disallow the previous syntax
```
  given (x: T)
    ... extension methods
```
Require instead
```
  given extension (x: T)
    ... extension methods
```
This avoids the confusion with given instances for types in parentheses and makes
i7515.scala compile. To allow building with 0.20rc1, we allow the old extension syntax
if it comes with a name. E.g. the following is still allowed, but will be dropped
once we have bootstrapped with this commit present.
```
  given ops: (x: T)
    ... extension methods
```
@odersky
Copy link
Contributor Author

odersky commented Nov 16, 2019

We could make individual extension methods also obey implicit scope. Why not?

It would introduce more coupling and make things more complicated. So I am against it in principle, but even more so now where we want to get to feature freeze. There's no time anymore for considering large and sweeping changes.

odersky added a commit to dotty-staging/dotty that referenced this pull request Nov 17, 2019
Allow `end given` as an end marker for anonymous givens.
These could not have an end marker before.

Based on scala#7557.
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

docs/docs/reference/contextual/extension-methods.md Outdated Show resolved Hide resolved
docs/docs/reference/contextual/extension-methods.md Outdated Show resolved Hide resolved
library/src/scala/tasty/reflect/ImplicitsOps.scala Outdated Show resolved Hide resolved
library/src/scala/tasty/reflect/ImplicitsOps.scala Outdated Show resolved Hide resolved
@nicolasstucki nicolasstucki merged commit 9c987cb into scala:master Nov 18, 2019
@nicolasstucki nicolasstucki deleted the given-extends branch November 18, 2019 15:52
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.

8 participants