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 3.1.2-RC1] Inferred union in type argument position gets widened #14494

Closed
ansvonwa opened this issue Feb 16, 2022 · 17 comments · Fixed by #15343
Closed

[Regression in 3.1.2-RC1] Inferred union in type argument position gets widened #14494

ansvonwa opened this issue Feb 16, 2022 · 17 comments · Fixed by #15343
Assignees
Labels
area:implicits related to implicits itype:bug regression This worked in a previous version but doesn't anymore
Milestone

Comments

@ansvonwa
Copy link
Member

ansvonwa commented Feb 16, 2022

Compiler version

3.1.2-RC1

First bad commit 3ab18a9 in #14026

Minimized code

object ImplNotFound:
  def main(args: Array[String]): Unit =
    val res: Seq[String | Int] = (??? : Seq[Int]).collect {
      case 1 => Seq("")
      case 2 => Seq(1)
    }.flatten

Output

[error] -- Error: /.../ImplNotFound.scala:6:13 
[error] 6 |    }.flatten
[error]   |             ^
[error]   |no implicit argument of type Seq[Matchable] => IterableOnce[B] was found for parameter asIterable of method flatten in trait IterableOps
[error]   |
[error]   |where:    B is a type variable with constraint <: String | Int

Expectation

Successful compilation as in 3.0.0 to 3.1.1

@ansvonwa ansvonwa added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 16, 2022
@romanowski romanowski added the regression This worked in a previous version but doesn't anymore label Feb 16, 2022
@romanowski romanowski added this to the 3.1.2 milestone Feb 16, 2022
@romanowski romanowski added area:implicits related to implicits and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 16, 2022
@odersky
Copy link
Contributor

odersky commented Feb 21, 2022

It would be good to identify the commit that caused this.

@ansvonwa
Copy link
Member Author

It is Create fresh type variables to keep constraints level-correct from #14026

@odersky
Copy link
Contributor

odersky commented Feb 21, 2022

@ansvonwa Thanks for the bisect! @smarter I guess that's for you then 😉

@smarter
Copy link
Member

smarter commented Feb 21, 2022

In 3.1.1 the following code used to infer a result type of Seq[Seq[String | Int]]:

object ImplNotFound:
  def main(args: Array[String]): Unit =
    val res = (??? : Seq[Int]).collect {
      case 1 => Seq("")
      case 2 => Seq(1)
    }
[[syntax trees at end of                     typer]] // try/i14494.scala
package <empty> {
  final lazy module val ImplNotFound: ImplNotFound = new ImplNotFound()
  final module class ImplNotFound() extends Object() {
    this: ImplNotFound.type =>
    def main(args: Array[String]): Unit =
      {
        val res: Seq[Seq[String | Int]] =
          (??? :Seq[Int]).collect[Seq[String | Int]](
            {
              def $anonfun(x$1: Int): Seq[String | Int] =
                x$1:(x$1 : Int) @unchecked match
                  {
                    case 1 =>
                      Seq.apply[String](["" : String]*)
                    case 2 =>
                      Seq.apply[Int]([1 : Int]*)
                  }
              closure($anonfun:PartialFunction[Int, Seq[String | Int]])
            }
          )
        ()
      }
  }
}

But now the union gets widened to Matchable:

package <empty> {
  final lazy module val ImplNotFound: ImplNotFound = new ImplNotFound()
  final module class ImplNotFound() extends Object() {
    this: ImplNotFound.type =>
    def main(args: Array[String]): Unit =
      {
        val res: Seq[Seq[Matchable]] =
          (??? :Seq[Int]).collect[Seq[Matchable]](
            {
              def $anonfun(x$1: Int): Seq[Matchable] =
                x$1:(x$1 : Int) @unchecked match
                  {
                    case 1 =>
                      Seq.apply[String](["" : String]*)
                    case 2 =>
                      Seq.apply[Int]([1 : Int]*)
                  }
              closure($anonfun:PartialFunction[Int, Seq[Matchable]])
            }
          )
        ()
      }
  }

No idea why.

@smarter
Copy link
Member

smarter commented Feb 21, 2022

I don't believe I'll have time to fix this for 3.1.2 (or any time before September of this year really!)

@smarter smarter removed this from the 3.1.2 milestone Feb 21, 2022
@smarter
Copy link
Member

smarter commented Feb 21, 2022

I guess the root of the problem is that type variable avoidance and widenUnion do not commute, as widenUnion is happy to widen any union it sees at the top-level when instantiating a type variable, and adding more type variables create more opportunity for it to run. Perhaps we can simply not run widenUnion when instantiating a type variable created by avoidance (which can be identified by checking for AvoidNameKind on its name)

@smarter
Copy link
Member

smarter commented Feb 21, 2022

In fact, it's not even that we have more type variables which causes issues, it's that we're instantiating them in a different order, given:

?A1 >: String
?A2 >: Int
?B >: Seq[?A1] | Seq[?A2]

If we instantiate ?A1 and ?A2 first we're left with ?B >: Seq[Int] | Seq[String] which leads to ?B := Seq[Int | String].
But if we instantiate ?B first, we get:

?B := Seq[?A1]
?A1 >: String | Int

And then of course we instantiate to ?A1 := Matchable. We could try to come up with some heuristic to change the order in which we do the instantiations, but that seems likely to break as many things as it fixes. Perhaps we should mark the inferred union String | Int to be a hard union since it came from comparing the arguments of two AppliedTypes?

@smarter
Copy link
Member

smarter commented Feb 21, 2022

Perhaps we should mark the inferred union String | Int to be a hard union since it came from comparing the arguments of two AppliedTypes?

This seems hard to track of. Perhaps we need to rethink widening on type variable instantiation. Can we get away with only widening soft unions when inferring the type of a definition and when performing an implicit search?

@smarter smarter changed the title Implicit resolution regression in 3.1.2-RC1 [Regression in 3.1.2-RC1] Inferred union in type argument position gets widened Feb 21, 2022
@smarter
Copy link
Member

smarter commented Feb 21, 2022

/cc @mbovel who's been on a crusade for less widening :)

@smarter
Copy link
Member

smarter commented May 2, 2022

Alternative idea: when we add the constraint ?B >: Seq[?A1] | Seq[?A2] perhaps at this point we could decide to unify ?A1 and ?A2 (we already do something like that for intersections in upper-bounds: https://github.com/lampepfl/dotty/blob/0e83a80fcc4713e45d45a0aa7b00d50ee5a3595d/compiler/src/dotty/tools/dotc/config/Config.scala#L103-L110), but we would ensure (via some piece of state) that any union we create at the top-level when doing this is a hard union.

smarter added a commit to dotty-staging/dotty that referenced this issue May 2, 2022
@anatoliykmetyuk anatoliykmetyuk added this to the 3.1.3 milestone May 30, 2022
@odersky
Copy link
Contributor

odersky commented May 31, 2022

@smarter Would it be possible to get the wip into 3.1.3? We are sort of blocked on this regression.

@smarter
Copy link
Member

smarter commented May 31, 2022

I'm not confident in what that wip is doing, I have no idea if it's sound and it might break as many things as it fixes. I'm more optimistic on the plan I outlined in #14770 (comment) which would make the logic that creates hard unions in constraints more robust in general.

@odersky
Copy link
Contributor

odersky commented May 31, 2022

Hmm. Should we revert #14026 then?

@smarter
Copy link
Member

smarter commented May 31, 2022

Possibly, though instead of reverting everything you probably only need to revert 3ab18a9 and the subsequent commits that removed workarounds that were no longer needed after that commit. Instead of reverting it might also be easier to turn off level-checking (make levelOK always return true)

@odersky
Copy link
Contributor

odersky commented May 31, 2022

Instead of reverting it might also be easier to turn off level-checking (make levelOK always return true)

OK, I am going to try that.

@odersky
Copy link
Contributor

odersky commented May 31, 2022

Unfortunately, this fails the bootstrap build. I get

[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/backend/sjs/ScopedVar.scala:32:24 
[error] 32 |    val stack = ass.map(_.push())
[error]    |                        ^^^^^^^^
[error]    |    Found:    (_$1: dotty.tools.backend.sjs.ScopedVar.Assignment[?]) => 
[error]    |      dotty.tools.backend.sjs.ScopedVar.AssignmentStackElement[_$1.T]
[error]    |    Required: dotty.tools.backend.sjs.ScopedVar.Assignment[?] => 
[error]    |      dotty.tools.backend.sjs.ScopedVar.AssignmentStackElement[_$1.T]
[error]    |
[error]    | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/dotc/config/Settings.scala:192:73 
[error] 192 |    def defaultState: SettingsState = new SettingsState(allSettings map (_.default))
[error]     |                                                                         ^^^^^^^^^
[error]     | Found:    (_$5: dotty.tools.dotc.config.Settings.Setting[?]) => _$5.T
[error]     | Required: dotty.tools.dotc.config.Settings.Setting[?] => _$5.T
[error]     |
[error]     | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/dotc/interactive/Interactive.scala:387:49 
[error] 387 |        symbol.ownersIterator.toList.reverse.map(_.name)
[error]     |                                                 ^^^^^^
[error]     |Found:    (_$13: dotty.tools.dotc.core.Symbols.Symbol) => _$13.ThisName
[error]     |Required: dotty.tools.dotc.core.Symbols.Symbol => _$13.ThisName
[error]     |
[error]     | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/dotc/sbt/ThunkHolder.scala:19:20 
[error] 19 |    toForce.foreach(_.get())
[error]    |                    ^^^^^^^
[error]    |                    Found:    (_$1: xsbti.api.Lazy[?]) => _$1.T | Null
[error]    |                    Required: xsbti.api.Lazy[?] => _$1.T | Null
[error]    |
[error]    | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/dotc/typer/CrossVersionChecks.scala:114:27 
[error] 114 |            concrOvers.map(_.name).mkString("    ", ", ", ""), tree.srcPos)
[error]     |                           ^^^^^^
[error]     | Found:    (_$4: dotty.tools.dotc.core.Symbols.Symbol) => _$4.ThisName
[error]     | Required: dotty.tools.dotc.core.Symbols.Symbol => _$4.ThisName
[error]     |
[error]     | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala:882:37 
[error] 882 |      val args = tp.args.mapconserve(arg =>
[error]     |                                     ^
[error]     |Found:    (arg: dotty.tools.dotc.ast.Trees.Tree[dotty.tools.dotc.ast.Trees.Untyped]) => 
[error]     |  arg.ThisTree[dotty.tools.dotc.core.Types.Type]
[error]     |Required: dotty.tools.dotc.ast.Trees.Tree[dotty.tools.dotc.ast.Trees.Untyped] => 
[error]     |  arg.ThisTree[dotty.tools.dotc.core.Types.Type]
[error] 883 |        val argTp = tp.typeOfArg(arg) match
[error] 884 |          case NoType => WildcardType
[error] 885 |          case tp => wildApprox(tp, theMap, seen, internal)
[error] 886 |        arg.withType(argTp))
[error]     |
[error]     | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/dotc/typer/RefChecks.scala:49:56 
[error] 49 |      val defaultMethodNames = defaultGetterNames map { _ replace {
[error]    |                                                        ^
[error]    |       Found:    (_$1: dotty.tools.dotc.core.Names.Name) => _$1.ThisName
[error]    |       Required: dotty.tools.dotc.core.Names.Name => _$1.ThisName
[error] 50 |        case DefaultGetterName(methName, _) => methName
[error] 51 |      }}
[error]    |
[error]    | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/dotc/typer/Typer.scala:1382:42 
[error] 1382 |                val protoArgs = args map (_ withType WildcardType)
[error]      |                                          ^^^^^^^^^^^^^^^^^^^^^^^
[error]      |Found:    (_$19: dotty.tools.dotc.ast.Trees.Tree[dotty.tools.dotc.ast.Trees.Untyped]) => 
[error]      |  _$19.ThisTree[dotty.tools.dotc.core.Types.Type]
[error]      |Required: dotty.tools.dotc.ast.Trees.Tree[dotty.tools.dotc.ast.Trees.Untyped] => 
[error]      |  _$19.ThisTree[dotty.tools.dotc.core.Types.Type]
[error]      |
[error]      | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/dotty/tools/dotc/typer/Typer.scala:3548:21 
[error] 3548 |                .map(sym => sym.name -> sym)
[error]      |                     ^^^^^^^^^^^^^^^^^^^^^^
[error]      |Found:    (sym: dotty.tools.dotc.core.Symbols.Symbol) => (sym.ThisName,
[error]      |  dotty.tools.dotc.core.Symbols.Symbol
[error]      |)
[error]      |Required: dotty.tools.dotc.core.Symbols.Symbol => (sym.ThisName, 
[error]      |  dotty.tools.dotc.core.Symbols.Symbol
[error]      |)
[error]      |
[error]      | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /Users/odersky/workspace/dotty/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala:3107:22 
[error] 3107 |      matchings.map { tup =>
[error]      |                      ^
[error]      |Found:    (tup: Tuple) => Tuple.Concat[Tuple, tup.type]
[error]      |Required: Tuple => Tuple.Concat[Tuple, (tup² : Tuple)]
[error]      |
[error]      |where:    tup  is a reference to a value parameter
[error]      |          tup² is a parameter in an anonymous function in method treeMatch
[error]      |
[error]      |
[error]      |Note: a match type could not be fully reduced:
[error]      |
[error]      |  trying to reduce  Tuple.Concat[Tuple, Tuple]
[error]      |  failed since selector  Tuple
[error]      |  does not match  case EmptyTuple => Tuple
[error]      |  and cannot be shown to be disjoint from it either.
[error]      |  Therefore, reduction cannot advance to the remaining case
[error]      |
[error]      |    case x1 *: xs1 => x1 *: scala.Tuple.Concat[xs1, Tuple]
[error] 3108 |        Tuple.fromIArray(typeHoles.map(typeHoleApproximation).toArray.asInstanceOf[IArray[Object]]) ++ tup
[error]      |
[error]      | longer explanation available when compiling with `-explain`
[error] 10 errors found
[error] (scala3-compiler-bootstrapped / Compile / compileIncremental) Compilation failed
[error] Total time: 42 s, completed May 31, 2022 6:04:03 PM

Any advice what we should do here?

@smarter
Copy link
Member

smarter commented May 31, 2022

You'll probably need to revert ae1b00d and 629006b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:implicits related to implicits 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.

6 participants