fix: disallow toplevel infix definitions for vals, vars, givens, methods and implicits #17994
Conversation
|
No worries at all, I'll fix the conficts as soon as possible (probably tomorrow morning) |
…ods and implicits Part of scala#17738
arainko
force-pushed
the
issue-17738-toplevel-infix
branch
from
891c0d4 to
3f3bd01
Compare
There was a problem hiding this comment.
I'm kind of on the edge about adding a new error message class since originally I wanted to go with
ModifierNotAllowedbut I found its error message misleading in this context […]
I am also not sure about this. Could we for example have ModifierNotAllowedForDefinition take an additional string argument that would detail the error and be displayed with -explain?
Or could TopLevelDefCantBeInfix extend ModifierNotAllowedForDefinition and share the same message ID?
I'll ask the core contributors what they think about it later today.
|
After discussing with the other contributors, we came to the conclusion that a single error message would be more appropriate here. Could you please add an argument with a default value to |
…fierNotAllowed with a custom explanation in its place
Sporarum
dismissed
their stale review
Comments addressed, but other remain, so can't be Accepted yet either
|
(That way any review can approve, instead of specifically me) |
| val defKind = | ||
| if sym.flags.is(Method) then "def" | ||
| else if sym.flags.is(Mutable) then "var" | ||
| else if sym.flags.is(Given) then "given" | ||
| else "val" | ||
| fail(ModifierNotAllowedForDefinition(Flags.Infix, s"a toplevel $defKind cannot be infix")) |
There was a problem hiding this comment.
| val defKind = | |
| if sym.flags.is(Method) then "def" | |
| else if sym.flags.is(Mutable) then "var" | |
| else if sym.flags.is(Given) then "given" | |
| else "val" | |
| fail(ModifierNotAllowedForDefinition(Flags.Infix, s"a toplevel $defKind cannot be infix")) | |
| fail(ModifierNotAllowedForDefinition(Flags.Infix, s"A top-level ${sym.showKind} cannot be infix.")) |
Looks like sym.showKind could be a good fit here.
Its implemented there: https://github.com/lampepfl/dotty/blob/2b391c82de1861d8ab0196e78dd2975539b8082a/compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala#L477-L502
Note that the check file would need to be updated as well as the names are not exactly the same as what you currently have.
There was a problem hiding this comment.
Minor: my suggestion also makes the explanation a full sentence (we don't have very coherent formatting for error messages, but it seems like full sentences for explanations is more common), and using top-level instead of toplevel as it is more common.
…ion into a full sentence.
|
Thanks again @arainko for your excellent work ! |
|
I think I should be the one thanking you for having so much patience with me ❤️ |
|
I've not needed patience with you ! And even if I had, I'm more than happy to do the work to make this place welcoming ^^ |
Disallow infix toplevel definitions for anything that is not a
class,typealias,match type,extension methodand atrait(andobjectsas well but these are handled in their own PR).I'm kind of on the edge about adding a new error message class since originally I wanted to go with
ModifierNotAllowedbut I found its error message misleading in this context henceToplevelDefCantBeInfix, I don't know the exact preferences when it comes to things like these so feel free to point out any irregularities 😄Part of #17738
Continuation of #17966