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

Good code is red. Java interop issue with collectors #19637

Closed
SimY4 opened this issue Feb 7, 2024 · 4 comments · Fixed by #19659
Closed

Good code is red. Java interop issue with collectors #19637

SimY4 opened this issue Feb 7, 2024 · 4 comments · Fixed by #19659
Assignees
Milestone

Comments

@SimY4
Copy link

SimY4 commented Feb 7, 2024

Compiler version

Reproducible on 3.4.0-RC4. This code works fine on 3.3.1

Minimized code

import java.util.stream.*
import java.util.function.*

val map: java.util.Map[String, String] = Stream.of("1", "2", "3").collect(Collectors.toMap(
  (s: String) => s,
  Function.identity(),
  {
    case ("1", "1") => "1"
    case (_, l) => l
  }
))

Output

Found:    ("1" : String)
Required: Nothing

Expectation

Should compile

@SimY4 SimY4 added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 7, 2024
@bishabosha
Copy link
Member

workaround:

import java.util.stream.*
import java.util.function.*

val map: java.util.Map[String, String] = Stream.of("1", "2", "3").collect(Collectors.toMap(
  (s: String) => s,
  Function.identity(),
  ((k: String, v: String) => (k, v) match {
    case ("1", "1") => "1"
    case (_, l) => l
  })
))

@bishabosha
Copy link
Member

bishabosha commented Feb 7, 2024

second workaround:

import java.util.stream.*
import java.util.function.*

val map: java.util.Map[String, String] = Stream.of("1", "2", "3").collect(Collectors.toMap(
  (s: String) => s,
  Function.identity(),
  {
    case ("1", "1") => "1"
    case (_, l) => l
  }: java.util.function.BinaryOperator[String]
))

@bishabosha bishabosha added area: type inference and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 7, 2024
@odersky
Copy link
Contributor

odersky commented Feb 8, 2024

Can we get a bisect on this?

@nicolasstucki
Copy link
Contributor

If we add the test in tests/pos/i19637.scala we can bisect by running the following command:

$ scala-cli project/scripts/bisect.scala -- --releases 3.3.0-RC1-bin-20221124-e25362d-NIGHTLY..  compile tests/pos/i19637.scala

In the logs we see

Last good release: 3.4.0-RC1-bin-20231113-0dfe593-NIGHTLY
First bad release: 3.4.0-RC1-bin-20231114-18ada51-NIGHTLY
6c556aa35a25a9aa785367354f0f96b7a1baa09c is the first bad commit
commit 6c556aa35a25a9aa785367354f0f96b7a1baa09c
Author: odersky <odersky@gmail.com>
Date:   Sat Oct 28 18:14:50 2023 +0200

    Improve type inference for functions like fold
    
    When calling a fold with an accumulator like `Nil` or `List()` one used to have add
    an explicit type ascription. This is now no longer necessary. When instantiating
    type variables that occur invariantly in the expected type of a lambda, we now replace
    covariant occurrences of `Nothing` in the (possibly widened) type of the accumulator
    with fresh type variables.
    
    The idea is that a fresh type variable in such places is always better than Nothing. For
    module values such as `Nil` we widen to `List[<fresh var>]`. This does possibly cause a new
    type error if the fold really wanted a `Nil` instance. But that case seems very rare,
    so it looks like a good bet in general to do the widening.

 .../dotty/tools/dotc/core/ConstraintHandling.scala |  12 +--
 compiler/src/dotty/tools/dotc/core/Types.scala     |  25 +++--
 .../src/dotty/tools/dotc/transform/TypeUtils.scala |  16 +++-
 .../src/dotty/tools/dotc/typer/Inferencing.scala   | 103 +++++++++++++++++----
 compiler/src/dotty/tools/dotc/typer/Typer.scala    |  13 ++-
 tests/pos/folds.scala                              |  34 +++++++

The issue started in 6c556aa

odersky added a commit to dotty-staging/dotty that referenced this issue Feb 9, 2024
The previous code tried to recursively apply the current instance
of FullyDefinedAccumulator to the prospective instance type. This
can have unforeseen side-effects, as i19637 shows. We now are more
conservative: We check that the prospective instance type is already
fully defined without the possibility to instantiate more type variables.
This still passes the test cases that type variable improvement solves
and avoids the problem with scala#19637.

Fixes scala#19637
odersky added a commit that referenced this issue Feb 9, 2024
The previous code tried to recursively apply the current instance of
FullyDefinedAccumulator to the prospective instance type. This can have
unforeseen side-effects, as i19637 shows. We now are more conservative:
We check that the prospective instance type is already fully defined
without the possibility to instantiate more type variables. This still
passes the test cases that type variable improvement solves and avoids
the problem with #19637.

Fixes #19637
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
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.

7 participants