Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Wrong match case selected #16252

Closed
Bersier opened this issue Oct 26, 2022 · 11 comments · Fixed by #16262
Closed

Wrong match case selected #16252

Bersier opened this issue Oct 26, 2022 · 11 comments · Fixed by #16262
Assignees
Labels
area:inline area:pattern-matching itype:bug prio:high regression This worked in a previous version but doesn't anymore

Comments

@Bersier
Copy link
Contributor

Bersier commented Oct 26, 2022

Compiler version

3.2.0

Minimized code

class Baz:
  inline def foo: Int = bar(zero)
  inline def bar(inline i: Int): Int = inline i match
    case 0 => 0
    case _ => 1
  private inline def zero = 0

println(Baz().foo)

Output

1

Expectation

0

See also #15893

@Bersier Bersier added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 26, 2022
@prolativ prolativ added area:pattern-matching area:inline and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 27, 2022
@prolativ
Copy link
Contributor

This used to work in 3.1.3.
This doesn't work in 3.2.2-RC1-bin-20221025-292e56f-NIGHTLY

@prolativ prolativ added the regression This worked in a previous version but doesn't anymore label Oct 27, 2022
@prolativ
Copy link
Contributor

The problem disappears if we replace inline def zero with inline val zero or inline i match with i match

@Kordyjan Kordyjan added this to the 3.2.2 backports milestone Oct 27, 2022
@KacperFKorban
Copy link
Member

Minimization without running

class Baz:
  inline def foo: Int = bar(zero)
  inline def bar(inline i: Int): Int = inline i match
    case 0 => 0
    case _ => scala.compiletime.error("XD")
  private inline def zero = 0

@main
def main =
  println(Baz().foo)

@prolativ
Copy link
Contributor

The regressing commit is 86bb972

@odersky
Copy link
Contributor

odersky commented Oct 29, 2022

How important is it? It was @nicolasstucki's commit, and I don't feel confident to propose an alternative. Do we need to revert #15075, reopening the other issue? Or can it wait? It's a difficult tradeoff.

@odersky
Copy link
Contributor

odersky commented Oct 30, 2022

In fact, this should never have worked. Or rather, it does only work if the functions are declared transparent inline. The reason is that

inline def zero = 0

reduces to 0: Int, not 0. And an Int scrutinee cannot distinguish between 0 and not 0.

Otherwise put, if zero could have selected the right match case, then changing the definition of zero to 1 would change the typing if the program. But there's a requirement for normal inline functions that they cannot do that.

@dwijnand
Copy link
Member

dwijnand commented Oct 30, 2022

It's a little confusing to me that "disjointness" is a factor in match type reduction but not in inline matches. Makes it more work to remember the different semantics. I'm guessing it's required or perhaps much preferred.

@odersky
Copy link
Contributor

odersky commented Oct 30, 2022

It's a little confusing to me that "disjointness" is a factor in match type reduction but not in inline matches.

I agree. That's something we should discuss.

@Bersier
Copy link
Contributor Author

Bersier commented Oct 30, 2022

Could someone explain the resolution? Is this behavior as expected? Why does private change the behavior?

@odersky
Copy link
Contributor

odersky commented Oct 30, 2022

It probably should not work with public methods either. There's a grey zone where the inline happens to reduce something because of some optimization or other. We might want to track these down and avoid them at some point, but it's tedious work for not much benefit. But in any case, if the method is not transparent, you can rely only on the result type, which is Int not 0.

@Bersier
Copy link
Contributor Author

Bersier commented Oct 31, 2022

@odersky thank you for clarifying.

A few more observations:

  1. This behavior (and how it differs from match type reduction) doesn't currently seem to be fully documented.

  2. One might naively assume the inline match to be equivalent to inline if i == 0 then 0 else 1 (but the latter leads to a compile-time error).

  3. In both cases, it also seems at odds with the part of the documentation that says It guarantees that inlining actually happens instead of being best-effort. According to that statement, one might naively assume code reduction to proceed somewhat as follows:

class Baz:
  inline def foo: Int = bar(zero)
  inline def bar(inline i: Int): Int = inline if i == 0 then 0 else 1
  private inline def zero = 0

println(Baz().foo)

--> (because zero is guaranteed to be inlined)

class Baz:
  inline def foo: Int = bar(0)
  inline def bar(inline i: Int): Int = inline if i == 0 then 0 else 1

println(Baz().foo)

--> (because bar is guaranteed to be inlined)

class Baz:
  inline def foo: Int = inline if 0 == 0 then 0 else 1

println(Baz().foo)

--> (inline if reduction)

class Baz:
  inline def foo: Int = 0

println(Baz().foo)

--> (because foo is guaranteed to be inlined)

println(0)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:inline area:pattern-matching itype:bug prio:high regression This worked in a previous version but doesn't anymore
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants