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

Refine overloading and implicit disambiguation #20084

Merged
merged 4 commits into from
Apr 8, 2024

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Apr 3, 2024

We sometimes have two alternatives a.m and b.m with the same symbol and type but different prefixes. Previously these would always be ambiguous. We now try to disambiguate this so that the alternative with the more specific prefix wins. To determine this, we widen prefixes also going from module classes to their parents and then compare the resulting types.

This might fix a problem in ScalaTest that popped up after #20054.

odersky added 3 commits April 3, 2024 20:41
We sometimes have two alternatives a.m and b.m with the same symbol
but different prefixes. Previously these would always be ambiguous.
We now try to disambiguate this so that the alternative with the
more specific prefix wins. To determine this, we widen prefixes
also going from module classes to their parents and then compare
the resulting types.

This might fix a problem in ScalaTest that popped up after scala#20054.
compareOwner did certain tests for one side but not for the other, which made its
outcome dependent on the order in which alternatives were presented.
The alternatives with the same symbol could have nevertheless different types. We first want
to disambiguate based on these types before we turn to prefixes as a final tie breaker.
@odersky odersky force-pushed the change-implicit-disambiguation branch 2 times, most recently from 714c774 to 8eeee0b Compare April 7, 2024 13:42
@odersky odersky requested a review from EugeneFlesselle April 7, 2024 13:42
@odersky odersky marked this pull request as ready for review April 7, 2024 13:42
@odersky
Copy link
Contributor Author

odersky commented Apr 7, 2024

It did not fix the ScalaTest problem but it's an improvement anyway.

Copy link
Contributor

@EugeneFlesselle EugeneFlesselle left a comment

Choose a reason for hiding this comment

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

The following example using an implicit parameter does not work as before

class I[X]
class J[X]

trait A:
  given I[B] = ???
  given (using I[B]): J[B] = ???
object A extends A

trait B extends A
object B extends B

//import B.given, A.given

def Test =
  summon[I[B]]
  summon[J[B]] // Error

I tried an alternative here using the prefix comparison in both cases.

compiler/src/dotty/tools/dotc/typer/Applications.scala Outdated Show resolved Hide resolved
Copy link
Contributor Author

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Latest commit LGTM

@odersky odersky merged commit d2a6392 into scala:main Apr 8, 2024
19 checks passed
@odersky odersky deleted the change-implicit-disambiguation branch April 8, 2024 20:44
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
odersky added a commit that referenced this pull request Oct 5, 2024
Fix #21669

This part of code is from #20084

We should check `parents` is non-empty before calling `reduceLeft`.

I haven't been able to create a standalone test. To test locally:

`i21669.scala`
```scala
//> using dep "com.softwaremill.sttp.tapir::tapir-sttp-client:1.11.5"

import sttp.tapir.*
import sttp.tapir.client.sttp.SttpClientInterpreter

@main def run =
    lazy val pingGET = endpoint.get
        .in("ping")
        .out(stringBody)

    SttpClientInterpreter()
        .toRequest(pingGET, Some(uri"http://localhost:8080"))
```

```
> sbt publishLocal
> scala compile --server=false -S 3.6.0-RC1-bin-SNAPSHOT i21669.scala
-- [E008] Not Found Error: /dotty/i21669.scala:12:33 ------
12 |        .toRequest(pingGET, Some(uri"http://localhost:8080"))
   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |value uri is not a member of StringContext, but could be made available as an extension method.
   |
   |One of the following imports might fix the problem:
   |
   |  import sttp.client3.UriContext
   |  import sttp.client3.quick.UriContext
   |  import sttp.model.Uri.UriContext
   |
1 error found
Compilation failed
```
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.

3 participants