Skip to content

Conversation

nox213
Copy link
Contributor

@nox213 nox213 commented May 2, 2025

closes #22989

Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

One note on how the feature should work, if it is a feature.

val p = pattern2(location)
if in.isColon then
val isVariableOrNumber = isVarPattern(p) || p.isInstanceOf[Number]
val isVariableOrNumber = isVarPattern(p) || p.isInstanceOf[Number] || isBackquoted(p)
Copy link
Contributor

Choose a reason for hiding this comment

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

Scala 2 honors the requirement even in backquoting.

scala> 42 match { case `Int`: Int => `Int` }
                            ^
       error: Pattern variables must start with a lower-case letter. (SLS 8.1.1.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Normal syntax for non-var: 42 match { case X @ (_: Int) => X }

Maybe it's worth adding a neg test.

@@ -0,0 +1,3 @@
//> using options -Xfatal-warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//> using options -Xfatal-warnings
//> using options -Werror


def i15784_auxiliary = 42 match
case `type` : Int => `type`
case _ => ???
Copy link
Contributor

Choose a reason for hiding this comment

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

You could quiet the other warnings and add the -Werror in the existing file. These directories get large.

By quiet, I'd suggest a def for each "rest" test. (and not just nowarn.)

@nox213 nox213 requested a review from som-snytt May 6, 2025 13:11
@nox213
Copy link
Contributor Author

nox213 commented May 6, 2025

Thanks for the review!

case X: Int => X // warn

def case3 = 42 match
case `Int`: Int => `Int` // warn
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the test rig checks the // warn in a neg test, but of course it's verified by the check file. (I'm wondering if the test rig should do that, as an enhancement. I know a difference is that it's OK to have zero warns in a warn test but not OK to have zero errors in a neg test.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, would it be better if I make a separate tests/warn/i15784.scala?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I think it's fine. Maybe with -Werror you can mark them error. But I was just musing aloud, in case I read this again later.

Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

Nice! I wouldn't have guessed unsplice.

@som-snytt
Copy link
Contributor

As linked above, I got a bonus Scala 2 ticket out of it.

@som-snytt som-snytt merged commit c85982c into scala:main May 7, 2025
29 checks passed
@WojciechMazur WojciechMazur added this to the 3.7.2 milestone Jun 18, 2025
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.

Incorrect warning on backticked typed pattern

4 participants