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

Fix errors in explicit type annotations in inline match cases #16257

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Oct 27, 2022

Previously, Unapply trees would have type bindings generated inside their body and this was the only case handled in InlineReducer. However, this mainly happened for inferred type parameters, and Unapply with an explicit binding inside a type annotation was not handled, leading to a "cannot reduce match" error. This case is now handled and a related comment was added as well.

Fixes #15971
For context, a more concise minimization then the one proposed in the issue above would be something like:

sealed trait A
case class B[T](x: T) extends A

object Test {
  inline def fun(x: A): Int = inline x match {
    case B(x1): B[t] => 0
  }

  @main def main() = 
    val x = B(0)
    fun(x)
}

And the issue itself ended up being unrelated to macros. I tried submitting this PR with the original minimization, however checker (-Ycheck) returned an exception, failing the tests. I tried to debug that as well, however I honestly spent an excessive amount of time on that, so for now I would like to at the very least, submit this fix for now, and reattempt fixing the checker later. For that reason I created a separate issue for the check failures, with a new minimization, in #16331.

Previusly, Unapply trees would have type bindings generated inside their
body and this was the only case handled in InlineReducer. However,
this happened for inferred type parameters, and Unapply with an
explicit binding inside a type annotation was not handled, leading to
a "cannot reduce match" error. This case is now handled and a related
comment was added as well.
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix LGTM, but note that in #16150 we're looking to disallowing the case B(x1): B[t] syntax.

@jchyb jchyb marked this pull request as ready for review November 14, 2022 10:48
@jchyb
Copy link
Contributor Author

jchyb commented Nov 14, 2022

@dwijnand Are you aiming for 3.3.0 with that? If so then is it better to merge this PR, close, or wait (honest question, I wouldn't want to cause any trouble and this PR applies to pretty much only case B(x1): B[t])

@dwijnand
Copy link
Member

Probably best to merge this one, so that either it works thanks to your fix or it's just another version of that code not compiling because the syntax is disabled. Yeah, I was aiming for 3.3.0 for that change.

@jchyb jchyb merged commit 0298581 into scala:main Nov 14, 2022
jchyb added a commit to jchyb/dotty that referenced this pull request Nov 17, 2022
PR scala#16257 introduced a test for bindings in type annotations.
Simultanously, scala#16150 disallowed the syntax that allowed the test to
parse and compile.
The test should be no longer needed.
@jchyb jchyb mentioned this pull request Nov 17, 2022
nicolasstucki added a commit that referenced this pull request Nov 18, 2022
Should fix CI.
PR #16257 introduced a test for handling bindings in type annotations
inside match cases. Simultaneously, #16150 disallowed the same syntax.
Because of that the test should no longer be necessary, as it only
pertains to the removed feature.
little-inferno pushed a commit to little-inferno/dotty that referenced this pull request Jan 25, 2023
PR scala#16257 introduced a test for bindings in type annotations.
Simultanously, scala#16150 disallowed the syntax that allowed the test to
parse and compile.
The test should be no longer needed.
@Kordyjan Kordyjan added this to the 3.3.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unneeded use of a binding fixes tuple-matching macro that should work without it
3 participants