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

Allow exports in extension clauses #14497

Merged
merged 7 commits into from
May 2, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Feb 16, 2022

This is part of a long term strategy to deprecate and remove general implicit conversions in Scala.

In the thread https://contributors.scala-lang.org/t/can-we-wean-scala-off-implicit-conversions/4388
we identified two areas where implicit conversions or implicit classes were still essential. One was
adding methods to new types in bulk. This can be achieved by defining an implicit class that inherits
from some other classes that define the methods. Exports in extension clauses provide a similar
functionality without relying on implicit conversions under the covers.

def m(c: C): Int = c.x + 1
export O.*
// generates
// type C = O.C

Choose a reason for hiding this comment

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

Is this enough to be able to do C(1) elsewhere? It reads like no to me since only the type is exported. Can we clarify that in the documentation and examples (show if it is allowed or disallowed explicitly)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One can allocate using type aliases (that's why we can write new StringBuilder for instance, because we have

  type StringBuilder = scala.collection.mutable.StringBuilder

in the Scala package. So that's a general principle, not related to exports.

Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

it would be nice if exports of an extension method could unify the parameters, as it stands the extension parameters are appended after, and parameter names are not sanitised:

package app

object StrSyntax {
  extension (s: String) {
    def x2 = s * 2
  }
}

object DeferredSyntax {
  extension (s: String) {
    private def syntax = StrSyntax
    export syntax.x2 //    ==>  extension (s: String) def x2(s: String): String  <--- there are 2 `s` parameters here?
  }
}

@main def Test = {
  locally {
    import DeferredSyntax.*
    val ref: String => String = "Hello, world!".x2 // can we drop the extension parameter list of `x2`?
  }
  locally {
    import StrSyntax.*
    val ref: String = "Hello, world!".x2
  }
}

Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

I will request changes due to the issue with hygiene of parameter names

@raquo
Copy link
Contributor

raquo commented Feb 19, 2022

Is it possible to express the StringOps example from this PR's docs in a library written in Scala 2.13 and cross-compiled to Scala 3? I mean, without end users seeing warnings / errors, and without requiring end users to import scala.language.implicitConversions at every use site?

Currently I use Scala 2 implicit conversions to a StringOps value class for that purpose. If the warning is not otherwise avoidable, then maybe the Scala 2 way of doing extension methods could be exempted from the warning for the time being?

@odersky
Copy link
Contributor Author

odersky commented Feb 19, 2022

@raquo Not sure I understand. If you need to cross-compile, you need to use Scala 2 implicit conversions anyway, and these don't produce a use site warning.

@raquo
Copy link
Contributor

raquo commented Feb 19, 2022 via email

@odersky odersky force-pushed the add-exports-in-extmethods branch 2 times, most recently from 42e470f to fe8a11c Compare February 19, 2022 12:07
@bishabosha bishabosha self-requested a review February 22, 2022 01:35
@bishabosha bishabosha added the needs-minor-release This PR cannot be merged until the next minor release label Feb 22, 2022
@bishabosha
Copy link
Member

This will need a minor bump as it is a source forward breaking change - I think this could be considered a breaking tasty change (previously it was expected that the prefix should be a stable path) - we should have a forward tasty compatibility test added

@odersky
Copy link
Contributor Author

odersky commented Feb 22, 2022

This will need a minor bump as it is a source forward breaking change

Yes, agreed.

I think this could be considered a breaking tasty change (previously it was expected that the prefix should be a stable path) - we should have a forward tasty compatibility test added

In fact, the stability requirement is only dropped for paths in extension methods, and exports are expanded out after desugar, so I don't see how this will affect Tasty. I won't have the time to do a forward compatibility test, and believe none is necessary. In any case this should not be a stopper for getting this merged.

@odersky odersky added this to the 3.2.0-RC1 milestone Feb 22, 2022
@bishabosha
Copy link
Member

Another question, should we avoid exporting extension methods (from inside extension block) using the export wildcard - it could be confusing - so best to require explicit export for them

@bishabosha
Copy link
Member

exports are expanded out after desugar, so I don't see how this will affect Tasty.

We kept exports in tasty since #10182, they are used by the doctool for example

@bishabosha
Copy link
Member

@odersky please may you check the note I added in 77447c8

@bishabosha bishabosha assigned odersky and unassigned bishabosha Feb 24, 2022
@odersky
Copy link
Contributor Author

odersky commented Feb 24, 2022

@bishabosha In fact, it might be better to simply exclude extensions from exports, as you suggested. I see not much use in treating them as normal methods with a new extension slapped on to them. That also opens up the possibility to unify the extension parameters in the future, if we so decide (although I think that future is unlikely since such unification looks like a can of worms).

@bishabosha
Copy link
Member

It might be better to simply exclude extensions from exports, as you suggested.

I'll add a commit for this

@odersky
Copy link
Contributor Author

odersky commented Feb 24, 2022

I am already on it.

@som-snytt
Copy link
Contributor

@adampauls lampepfl/dotty-feature-requests#148 and #9704

@odersky
Copy link
Contributor Author

odersky commented Apr 18, 2022 via email

@adampauls
Copy link
Contributor

Well, there is definitely a separate something. Note that this works just fine:

class Outer(x: Int) {self =>
  def show(param1: Int, param2: Int): String = (x + param1 + param2).toString
  class Inner()

  extension (i: Inner)
    private def __this = self
    def show(param1: Int, param2: Int): String = self.show(param1, param2)
    //export __this.show
}

def use: String = {
  val o = new Outer(1)
  val i = new o.Inner()
  i.show(3, 4)
}

I can write show in two different places, but I can't export it from one to the other.

@adampauls
Copy link
Contributor

And I guess to complete the thought, this does not work

class Outer(x: Int) {self =>
  def show(param1: Int, param2: Int): String = (x + param1 + param2).toString
  def show(i: Inner)(param1: Int, param2: Int): String = show(param1, param2)
  class Inner()

  extension (i: Inner)
    private def __this = self
    def show(param1: Int, param2: Int): String = self.show(param1, param2)
    //export __this.show
}

I think this argues for it being okay to simply allow the export and let the erasure check catch any conflicts later, as it already does.

But I now understand that this would at a minimum be a follow-on PR, so I'll stop the chatter here. Thanks for the clarification!

@adampauls
Copy link
Contributor

To complete the thought: you currently can export functions that already have an overload defined in the export scope:

object Other:
  def show(param1: Int): String = "x"

class Outer(x: Int):
  def show(param1: Int, param2: Int): String = (x + param1 + param2).toString
  export Other.show

def use: String = {
  val o = new Outer(1)
  o.show(3)
  o.show(3, 4)
}

And of course if Other.show has the same signature as Outer.show (after erasure), there will be an error. Seems reasonable then to allow exports inside extensions too?

@bishabosha
Copy link
Member

bishabosha commented Apr 20, 2022

@odersky this should be rebased after #14963 to use the correct path type as the prefix for default getters

@som-snytt
Copy link
Contributor

@adampauls That would make an interesting thread on Discussions.

The restriction on eligibility of members for export has to do with the "motivation" in the reference, which is eloquent on composition over inheritance. Arguably, the pattern you advocate risks greater coupling. Also, export allows me to reason about symbols, and not about the erasure of signatures, and also not just about names: for example, beginners ask, If I import X.f and also import Y.f, why isn't f just overloaded? By contrast, exports can introduce an overloaded symbol.

Extension methods are also a tool for managing a name space, i.e., a scope. They introduce methods that are special with respect to applicability: how to invoke them, and the scope that makes them available. At the intersection of these two features, am I aware of f as a special kind of method? It is no longer f from the enclosing class but "reborn"? I am about to wax poetic with the language of Easter. But export is an alias, the way thieves use aliases for scams and deceptions. They are not to be trusted but must be vetted.

I'd be interested in reading Discussions on these topics from people using the features.

odersky and others added 7 commits April 25, 2022 16:38
This is part of a long term strategy to get deprecate and remove general implicit conversions in Scala.

In the thread https://contributors.scala-lang.org/t/can-we-wean-scala-off-implicit-conversions/4388
we identified two areas where implicit conversions or implicit classes were still essential. One was
adding methods to new types in bulk. This can be achieved by defining an implicit class that inherits
from some other classes that define the methods. Exports in extension methods provide a similar
functionality without relying on implicit conversions under the covers.
Revert "Add note on export of extensions"

This reverts commit 77447c8.
@bishabosha bishabosha self-requested a review April 25, 2022 15:51
@odersky odersky merged commit aad399d into scala:main May 2, 2022
@odersky odersky deleted the add-exports-in-extmethods branch May 2, 2022 10:30
@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label May 2, 2022
@bishabosha
Copy link
Member

bishabosha commented May 11, 2022

to check before the release, should this only be under an experimental language feature import? @sjrd @julienrf

@sjrd
Copy link
Member

sjrd commented May 11, 2022

It should ideally be under an experimental import until SIP approval. Until it's officially back on its feet, though, I have bigger fights to fight; I would expect this change to be reasonably uncontroversial. Not that my opinion actually matters ...

@deusaquilus
Copy link
Contributor

I was really, really enthusiastic about this capability but upon further examination it doesn't seem to help me at all 😢.
Here's an overview: https://contributors.scala-lang.org/t/proposed-changes-and-restrictions-for-implicit-conversions/4923/167?u=deusaquilus

KacperFKorban added a commit that referenced this pull request Nov 21, 2022
in the thread
https://contributors.scala-lang.org/t/proposed-changes-and-restrictions-for-implicit-conversions/4923
we discussed two changes that would make implicit conversions largely
redundant. One proposed change was implemented in #14497. The other is
implemented here. It's based on #14497.

The idea of this PR is to have some way to specify that a method
parameter accepts implicit conversions on its arguments. Then the
feature warning on implicit conversion use would be suppressed in this
case. In the previous thread I proposed
`~` to mark convertible arguments but I now feel this is too cryptic.
Instead there is a `into` prefix in the
parameter type. E.g.
```scala
  def f(x: into T) = ...
```
For Scala 2, we introduce an annotation on the parameter that expresses
the same thing:
```scala
  // proposed Scala-2 syntax:
  def f(@allowConversions x: T) = 
```
A larger example:
```scala
given Conversion[String, Text] = Text(_)

@main def Test =

  def f(x: into Text, y: => into Text, zs: into Text*) =
    println(s"${x.str} ${y.str} ${zs.map(_.str).mkString(" ")}")

  f("abc", "def")  // ok, no feature warning
  f("abc", "def", "xyz", "uvw")  // also ok
  f("abc", "def", "xyz", Text("uvw"))  // also ok
```
A larger example is a parser combinator library that works without
unrestricted implicit conversions:

[tests/run/Parser.scala](https://github.com/lampepfl/dotty/blob/0f113da1bacc01603740cde4a5bafbfae39895f0/tests/run/Parser.scala)
@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.2.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants