Skip to content

Separate compilation breaks extension methods with the same name #16920

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

Closed
Ichoran opened this issue Feb 15, 2023 · 6 comments · Fixed by #17050
Closed

Separate compilation breaks extension methods with the same name #16920

Ichoran opened this issue Feb 15, 2023 · 6 comments · Fixed by #17050

Comments

@Ichoran
Copy link

Ichoran commented Feb 15, 2023

Compiler version

3.2.2 (and still in 3.3.0-RC2)

Minimized code

Scastie: https://scastie.scala-lang.org/Ichoran/fgRflBYNRciNP6bBG9WtSw/14

object One:
  extension (s: String)
    def wow: Unit = println(s)

object Two:
  extension (i: Int)
    def wow: Unit = println(i)

object Three:
  extension (s: String)
    def wow: Unit = println(s)
  extension (i: Int)
    def wow: Unit = println(i)

object Four:
  implicit class WowString(s: String):
    def wow: Unit = println(s)

object Five:
  implicit class WowInt(i: Int):
    def wow: Unit = println(i)

object Compiles:
  import Three._
  def test: Unit =
    5.wow
    "five".wow

object AlsoCompiles:
  import Four._
  import Five._
  def test: Unit =
    5.wow
    "five".wow

object Fails:
  import One._
  import Two._
  def test: Unit =
    5.wow
    "five".wow

Output

value wow is not a member of Int.
An extension method was tried, but could not be fully constructed:

    Playground.Two.wow(5)    failed with

        Reference to wow is ambiguous,
        it is both imported by import Playground.One._
        and imported subsequently by import Playground.Two._

value wow is not a member of String.
An extension method was tried, but could not be fully constructed:

    Playground.Two.wow("five")    failed with

        Reference to wow is ambiguous,
        it is both imported by import Playground.One._
        and imported subsequently by import Playground.Two._

Expectation

Extension methods work when compiled independently for different argument types (in different namespaces).

Thus, extension methods should be equally usable when compiled independently or jointly, and also are equally as usable as the implicit class extension mechanism.

Actual Behavior

Independent compilation does not work with extension methods when the extension methods have the same name because the import ambiguity mechanism vetoes the successfully-resolved method call. (Note that the error message tells you that it has actually fully successfully dispatched the method based on type.)

This prevents removal of the implicit class mechanism for extension methods, because implicit class does not have this problem.

(It seems that this problem should have been reported already by now, but I can't find it. However, if this is not fixed, to be safe, nobody should use extension methods in any library, because it is impossible to predict when a collision might occur with some other library extending some other class. It's particularly acute with Java classes where you might want to define some obvious operation like +, but someone might want to define it on Duration and someone else might want to define it on Path.)

@Ichoran Ichoran added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 15, 2023
@soronpo
Copy link
Contributor

soronpo commented Feb 15, 2023

It's not separate compilation. It's a separate namespace and import from that namespace. Unfortunately (IMO), this is the intended design. See https://contributors.scala-lang.org/t/change-shadowing-mechanism-of-extension-methods-for-on-par-implicit-class-behavior/5831
EDIT: I see you replied on that thread.

@soronpo
Copy link
Contributor

soronpo commented Feb 15, 2023

To move this thing ahead I recommend writing a SIP. This cannot be resolved via bug reports. I currently do not have enough time to formulate it, but will support any good proposal that would bring this matter to a close.

@WojciechMazur WojciechMazur added area:extension-methods and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 15, 2023
@Ichoran
Copy link
Author

Ichoran commented Feb 15, 2023

@soronpo - Haha, I'd forgotten that I wrote that before (was recovering from Covid back then, and my memory was really rubbish then).

I suppose you're right that this requires a SIP.

However, as they stand now, they're not extension methods, they're extension functions--the difference being that methods know how to dispatch based on the type of the first argument, and functions don't. The current situation is roughly as sensible as losing the ability to call length on a String when you import scala.collection.immutable.* because those classes have unrelated length functions.

And I think it is from a user experience actually an issue of independent compilation because you can get around the problem when compiling together by putting everything in the same scope. This is impossible when you compile independently.

So it may be a SIP-level bug, but I think it still should be considered to have the status of a bug, because the more you use extension methods, the less usable anything becomes. As you import libraries which use extension methods, you get quadratically bad name collisions, and unlike functions which you can still use by giving more of the path to the full namespace, extension methods are unusable as methods.

@Ichoran
Copy link
Author

Ichoran commented Feb 15, 2023

This bug as-a-bug could be resolved by a documentation change wherever extension methods are described, warning people to not use this mechanism and use implicit class instead, if the code might be consumed by anyone else.

Leaving it undocumented invites a minefield of problems as people write libraries with extension, people import more than one such library, and all the names of methods in common become unusable.

odersky added a commit to dotty-staging/dotty that referenced this issue Mar 6, 2023
Add a special case to name resolution so that when expanding an
extension method from `e.m` to `m(e)` and `m` is imported by several
imports on the same level, we try to typecheck under every such import
and pick the successful alternative if it exists and is unambiguous.

Fixes scala#16920
odersky added a commit to dotty-staging/dotty that referenced this issue Mar 6, 2023
Add a special case to name resolution so that when expanding an
extension method from `e.m` to `m(e)` and `m` is imported by several
imports on the same level, we try to typecheck under every such import
and pick the successful alternative if it exists and is unambiguous.

Fixes scala#16920
odersky added a commit to dotty-staging/dotty that referenced this issue Mar 28, 2023
Add a special case to name resolution so that when expanding an
extension method from `e.m` to `m(e)` and `m` is imported by several
imports on the same level, we try to typecheck under every such import
and pick the successful alternative if it exists and is unambiguous.

Fixes scala#16920
odersky added a commit to dotty-staging/dotty that referenced this issue Apr 22, 2023
Add a special case to name resolution so that when expanding an
extension method from `e.m` to `m(e)` and `m` is imported by several
imports on the same level, we try to typecheck under every such import
and pick the successful alternative if it exists and is unambiguous.

Fixes scala#16920
odersky added a commit that referenced this issue Apr 22, 2023
Add a special case to name resolution so that when expanding an
extension method from `e.m` to `m(e)` and `m` is imported by several
imports on the same level, we try to typecheck under every such import
and pick the successful alternative if it exists and is unambiguous.

Fixes #16920
@julienrf
Copy link
Contributor

julienrf commented May 9, 2023

Hey @Ichoran, did you have a chance to try the fix? It is available in nightlies with the import scala.language.experimental.relaxedExtensionImports.

@Ichoran
Copy link
Author

Ichoran commented May 10, 2023

@julienrf - Yes, it works! Yay!

But it only works if I use the language import before I do the other imports. (That confused me for a moment.)

However, the error messages--as usual for extensions--aren't always the best.

scala> import kse.flow.{given, _}; import kse.maths.{given, _}; import kse.maths.packed.{given, _}; import kse.eio.{given, _}
                                                                                
scala> "build.sc".path.exists
val res0: Boolean = true
                                                                                
scala> 5.orAlt[String].exists
-- [E008] Not Found Error: -----------------------------------------------------
1 |5.orAlt[String].exists
  |^^^^^^^^^^^^^^^^^^^^^^
  |value exists is not a member of Int Or String.
  |An extension method was tried, but could not be fully constructed:
  |
  |    kse.eio.exists(kse.flow.orAlt[Int](5)[String])
  |
  |    failed with:
  |
  |        Found:    Int Or String
  |        Required: java.nio.file.Path
1 error found
                                                                                
scala> 5.orAlt[String].exists(_ > 0)
val res1: Boolean = true

Anyway, it's a major improvement! I can, if I want, drop the implicit class / extends AnyVal method of creating extensions at this point.

Any chance it will get into 3.3.0 final?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants