Unhelpful error message when trying to use named extraction, when not matching case class or named tuple #23354
Conversation
|
The new message is emitted for which is the unnamed tuple case in the failing test. I wonder if the text could say "this is not a named tuple". I would be confused and say I thought they supported named tuples now. The PR doesn't have the test from the OP. The phrase "selector type" might be confusing. I think of In the OP, the selector expression was I don't know if there is a term that is precise yet friendly. |
|
Thanks @som-snytt, we (@jan-pieter , @RoccoMathijn @nmcb ) have had another look and we think we have improved the logic and error message a bit. Next up is adding and fixing tests, and optionally looking at the underlined text in the error message. My colleagues are currently looking into that. |
… pattern Co-authored-by: nmcb <[email protected]> Co-authored-by: jan-pieter <[email protected]> Co-authored-by: wwbakker <[email protected]> Co-authored-by: thijsnissen <[email protected]> Co-authored-by: RoccoMathijn <[email protected]>
|
@som-snytt what do you think, better now? |
| case _ => | ||
| report.error(em"No element named `$name` is defined in selector type $pt", arg.srcPos) | ||
| reordered.toList | ||
| val isCaseClass = pt.classSymbol.is(CaseClass) && !defn.isTupleClass(pt.classSymbol) |
There was a problem hiding this comment.
I see this variable is used twice, but it may not be needed. It could be inline def to avoid a call.
Otherwise, if it's a val, then the boolean exprs should always use it first and short-circuit the other computation.
if isCaseClass && selNames.isEmpty.
There was a problem hiding this comment.
Changed it to def as suggested.
Also updated order (doesn't really matter now, but maybe for if it is every changed to val or something)
| val nameToIdx = selNames.zipWithIndex.toMap | ||
| val reordered = Array.fill[untpd.Tree](selNames.length): | ||
| untpd.Ident(nme.WILDCARD).withSpan(wildcardSpan) | ||
| for case arg@NamedArg(name: TermName, _) <- elems do |
There was a problem hiding this comment.
I'm curious if it was intended to remove spaces around arg @ NamedArg. One also sees 2+2, I think that is an Ichorism, although I would not. Does arg@p convey something about precedence? I'm aware that the name binds to the entirety of the subsequent pattern. I don't disapprove spaceless; I might use it for x@_.
There was a problem hiding this comment.
There was not really a reason for it other than personal style preference. Changed it back, also to be consistent with the formatting on line 1789.
What is an Ichorism? Tried to google and chatgpt it, but they didn't know.
| -- [E215] Pattern Match Error: tests/neg/i22903.scala:18:21 ------------------------------------------------------------ | ||
| 18 | case ProductMatch(someName = x) => println (x) // error | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | Named patterns cannot be used with CustomProduct, because it is not a named tuple or case class |
There was a problem hiding this comment.
Very nice! And you solved the hardest problem of naming in the usual and best way, by omitting the name. If I were confused by the message, I would look at ProductMatch (supposing I had no idea what CustomProduct is); if I were still confused, the important part is that I can't use named patterns for some reason. It has something to do with CustomProduct, though we haven't named the role it plays.
Fixes #22903
Worked on together by @jan-pieter @nmcb @RoccoMathijn @thijsnissen and me.
PR should still be finished.