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

Ambigious types errors by picking nested/derived type and type defined in the same package #17433

Closed
WojciechMazur opened this issue May 8, 2023 · 8 comments · Fixed by #17438 · May be fixed by #17439
Closed

Ambigious types errors by picking nested/derived type and type defined in the same package #17433

WojciechMazur opened this issue May 8, 2023 · 8 comments · Fixed by #17438 · May be fixed by #17439
Assignees
Labels
itype:bug regression This worked in a previous version but doesn't anymore stat:needs minimization Needs a self contained minimization
Milestone

Comments

@WojciechMazur
Copy link
Contributor

Compiler version

3.3.1-RC1-bin-20230504-0e00420-NIGHTLY

Found in Community Build in following projects:

Project Version Build URL Notes
endpoints4s/endpoints4s 1.9.0 Open CB logs
sangria-graphql/sangria 4.0.0-RC4 Open CB logs
zio/zio-prelude 1.0.0-RC18 -> 1.0.0-RC19 Open CB logs

Output

In this case Value is also defined as sealed trait Value in other file

Error:  -- [E049] Reference Error: /build/repo/modules/core/src/main/scala/sangria/ast/AstVisitorCommand.scala:6:30 
Error:  6 |  val Skip, Continue, Break = Value
Error:    |                              ^^^^^
Error:    |                    Reference to Value is ambiguous.
Error:    |                    It is both defined in package sangria.ast
Error:    |                    and inherited subsequently in object AstVisitorCommand
Error:    |

Expectation

Should compile

@WojciechMazur WojciechMazur added itype:bug stat:needs minimization Needs a self contained minimization regression This worked in a previous version but doesn't anymore stat:needs triage Every issue needs to have an "area" and "itype" label labels May 8, 2023
@odersky
Copy link
Contributor

odersky commented May 8, 2023

I think it's due to #17033. Maybe revert that and check again?

@lrytz
Copy link
Member

lrytz commented May 8, 2023

Seems to be a bug, the ambiguity should only show if sangria.ast.Value is defined in the same file, which doesn't seem to be the case. https://github.com/sangria-graphql/sangria/blob/main/modules/core/src/main/scala/sangria/ast/AstVisitorCommand.scala

@lrytz
Copy link
Member

lrytz commented May 8, 2023

Probably best to revert it, I'll take a look to submit a fixed version.

@som-snytt
Copy link
Contributor

@lrytz maybe just a tweak

@@ -438,7 +438,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
               val found =
                 if owner.is(Package) then
                   owner.denot.asClass.membersNamed(name)
-                    .filterWithPredicate(d => !d.symbol.is(Package) && d.symbol.source == denot.symbol.source)
+                    .filterWithPredicate(d => !d.symbol.is(Package) && d.symbol.source.ne(util.NoSource) && d.symbol.source == denot.symbol.source)
                 else
                   val scope = if owner.isClass then owner.info.decls else outer.scope
                   scope.denotsNamed(name)

I tried it with sangria. Yes, it "pairs well" with sangria!

@lrytz
Copy link
Member

lrytz commented May 8, 2023

Thank you @som-snytt, that looks good to me. I'll PR it tomorrow if you didn't already.

@som-snytt
Copy link
Contributor

I'll grant you the honors, like King Charles III, or I guess in Scala it's King Charless. Oh, the pun is also Char-less which I pronounce "care less" as in "I could care less".

zio-prelude is honest ambiguity. I added some this. but it is tiresome.

@som-snytt
Copy link
Contributor

endpoints4s (https://github.com/endpoints4s/endpoints4s/blob/master/algebras/algebra/src/main/scala/endpoints4s/algebra/ChunkedEntities.scala#L57)

looks obscure to this reader. The intuition may have been that the type parameter is disambiguating.

Perhaps it's a good time to try out the new meme: “Borges thinks you should try a little harder.”

trait Chunks {

  /** A stream of chunks of type `A`.
    *
    * @tparam A Information carried by each chunk
    * @group types
    */
  type Chunks[A]
}

/** @group algebras */
trait ChunkedRequestEntities extends Chunks {
  this: EndpointsWithCustomErrors =>

  /** A request entity carrying chunks of `String` values
    *
    * @group operations
    */
  def textChunksRequest: RequestEntity[Chunks[String]]   // ambiguous Chunks

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue May 9, 2023
@nicolasstucki nicolasstucki removed the stat:needs triage Every issue needs to have an "area" and "itype" label label May 9, 2023
@som-snytt
Copy link
Contributor

@lrytz I took another look, in case it helps. at #17439 Feel free to disregard. I was just staring at the spinning gears. I had not crafted a test yet, only trying it locally.

@nicolasstucki nicolasstucki assigned lrytz and unassigned nicolasstucki May 22, 2023
sjrd added a commit that referenced this issue Jun 12, 2023
Kordyjan pushed a commit to dotty-staging/dotty that referenced this issue Jul 11, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
itype:bug regression This worked in a previous version but doesn't anymore stat:needs minimization Needs a self contained minimization
Projects
None yet
6 participants