Just warn on type ascription on a pattern #17454
Conversation
prolativ
force-pushed
the
pattern-type-ascription-warning
branch
from
cc452af
to
6d8d23b
Compare
|
Examples of problematic projects from OCB:
|
|
This PR undid the changes in the syntax specification as introduced in the mentioned PR, indicating that the parser should not directly raise an error for some type ascriptions on patterns, but as spotted in #17443 the specification might need to be updated further in a different way to make things consistent |
…n other than a variable or a number literal. This partially reverts the changes from scala#16150. This change is motivated by not breaking source compatibility for a number of projects in the Open Community Build.
prolativ
force-pushed
the
pattern-type-ascription-warning
branch
from
6d8d23b
to
b65ef59
Compare
There was a problem hiding this comment.
I am not sure what the warning should express. Is it an implementation restriction? (which would mean we try to lift it in the future). In this case it should be labelled "Implementation restriction: ". Or is it something that will be an error in the future? In that case we should make it an errorOrMigrationWarning instead.
That also affects syntax. If it is something we want to restrict in the future we should keep the current syntax around. Parsing could well be different from syntax. I think it's friendlier to accept Pattern2 on the LHS but then reject semantically, than to implement the restricted syntax verbatim.
| if in.isColon then | ||
| val isVariableOrNumber = isVarPattern(p) || p.isInstanceOf[Number] | ||
| if !isVariableOrNumber then | ||
| warning(em"Only variable and number literal patterns can have type ascriptions") |
There was a problem hiding this comment.
We should have a proper test for this. tests/neg/t5702-neg-bad-and-wild.check does not count since it is a syntax error in other ways already.
There was a problem hiding this comment.
I think tests/neg-custom-args/fatal-warnings/i15893.scala and tests/neg-custom-args/fatal-warnings/i10994.scala are enough as a test.
There was a problem hiding this comment.
I overlooked i10994.scala. Yes, agreed, that's enough.
Yes, that's the case. This should be an error, and we have made it an error. The problem is we were too eager, and this should have first gone through the stage of being a warning, as multiple libraries are using the forbidden syntax. I have nothing against reverting this PR soon and going back to reporting an error in 3.4.0 |
|
So then it should be an |
|
I think the changes should then be:
|
…uage specification * Make the parser's warning a migration warning
prolativ
force-pushed
the
pattern-type-ascription-warning
branch
from
8a6bf55
to
07c395b
Compare
Kordyjan
added
the
backport:nominated
Kordyjan
added
backport:done
Raise a warning instead of an error for a type ascription on a pattern other than a variable or a number literal.
This partially reverts the changes from #16150.
This change is motivated by not breaking source compatibility for a number of projects in the Open Community Build.