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

Regression in kalin-rudnicki/harness #19415

Closed
WojciechMazur opened this issue Jan 10, 2024 · 7 comments · Fixed by #19922
Closed

Regression in kalin-rudnicki/harness #19415

WojciechMazur opened this issue Jan 10, 2024 · 7 comments · Fixed by #19922
Assignees
Labels
area:typer itype:bug regression This worked in a previous version but doesn't anymore
Milestone

Comments

@WojciechMazur
Copy link
Contributor

Compiler version

3.4.0-RC1
First bad release: 3.3.2-RC1-bin-20230619-a68568c-NIGHTLY
Bisect points to b787d3c

Not affecting 3.3.2-RC1

Minimized code

def Test = {
  val left: Parser[String] = ???
  val right: Parser[Int] = ???
  val both = left && right

  val works = both.map(Ior.Both.apply)
  val fails = (left && right).map(Ior.Both.apply)
}

trait Parser[T]:
  final def &&[T2](other: Parser[T2])(implicit zip: Zip[T, T2]): Parser[zip.Out] = ???
  final def map[T2](f: T => T2): Parser[T2] = ???

infix trait Ior[+A, +B]
object Ior:
  final case class Both[+A, +B](a: A, b: B) extends (A Ior B)

trait Zip[In1, In2]:
  type Out

object Zip {
  type Out[In1, In2, O] = Zip[In1, In2] { type Out = O }
  implicit def zip2[_1, _2]: Zip.Out[_1, _2, (_1, _2)] = ???
}

Output

 |def Test = {
  |^
  |Recursion limit exceeded.
  |Maybe there is an illegal cyclic reference?
  |If that's not the case, you could also try to increase the stacksize using the -Xss JVM option.
  |For the unprocessed stack trace, compile with -Yno-decode-stacktraces.
  |A recurring operation is (inner to outer):
  |
  |  check fully defined A
  |  check fully defined (A, B)
  |  check fully defined Tuple.Elem[(A, B), (param)1]
  |  check fully defined A
  |  check fully defined (A, B)
  |  check fully defined Tuple.Elem[(A, B), (param)1]
  |  check fully defined A
  |  check fully defined (A, B)
  |  check fully defined Tuple.Elem[(A, B), (param)1]
  |  check fully defined A
  |  ...
  |
  |  check fully defined A
  |  check fully defined (A, B)
  |  check fully defined Tuple.Elem[(A, B), (param)1]
  |  check fully defined A
  |  check fully defined (A, B)
  |  check fully defined Tuple.Elem[(A, B), (param)1]
  |  check fully defined A
  |  check fully defined (A, B)
  |  check fully defined (A, B)
  |  check fully defined Tuple.Elem[(A, B), (param)1]
7 errors found
Compilation failed

Expectation

It probably should compile

@WojciechMazur WojciechMazur added itype:bug area:typer regression This worked in a previous version but doesn't anymore labels Jan 10, 2024
@WojciechMazur WojciechMazur added this to the 3.4.0 milestone Jan 11, 2024
@nicolasstucki
Copy link
Contributor

This issue started in d6c7e18. In this commit the issue is an infinite loop.

b787d3c broke the compiler in some other way. This is because the PR had many commits and some of them where broken. This breaks the assumptions needed to do an automatic bisection. @liufengyun make sure that your commits are squashed before merging them.

@nicolasstucki
Copy link
Contributor

The problematic code came from #18130

@nicolasstucki
Copy link
Contributor

The issue happens after the call to constrainResult(<scala.NonEmptyTuple.apply>, <Tuple.Elem[This, (param)2]>, <B>) in

https://github.com/lampepfl/dotty/blob/fff24b1fbae2a009aac5cfec136b899feaab0758/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala#L126

The following fixes this particular issue, but I am not sure if it is the correct way to fix it.

- constrainResult(mt, pt)
+ constrainResult(mt, wildApprox(pt))

@odersky
Copy link
Contributor

odersky commented Jan 27, 2024

Why was this assigned to me?

@odersky
Copy link
Contributor

odersky commented Jan 27, 2024

The commit in #18130 contains this code:

    def constrainResult(meth: Symbol, mt: Type, pt: Type)(using Context): Boolean =
      if (Inlines.isInlineable(meth)) {
        // Stricter behaviour in 3.4+: do not apply `wildApprox` to non-transparent inlines

So it seems not doing the wildApprox is fully intentional. We can't fix the problem by re-introducing it.

@odersky
Copy link
Contributor

odersky commented Jan 27, 2024

Seeing that #17924 is at the root of this, I'd like to propose a strategy; Let's all stop fixing any perceived shortcomings related to implicit conversions. Implicit conversions are too finely balanced, anything we do will likely break other stuff (as it did in this case). Also, we really should work hard to get rid of implicit conversions instead of going deeper into the rabbit hole.

@odersky
Copy link
Contributor

odersky commented Jan 27, 2024

I propose to revert both #18130 and #17924.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typer itype:bug regression This worked in a previous version but doesn't anymore
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants