-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Correctly detect colon lambda eol indent for optional brace of argument #22477
Correctly detect colon lambda eol indent for optional brace of argument #22477
Conversation
d8ff0e2
to
b8316d8
Compare
b8316d8
to
5ad196e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fantastic! i would select approve, mainly based on the tests :)
@mbovel This was tricky to figure out without knowing anything, but turns out to be a one-liner (plus a method) that may be a quick review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dream of simpler indentation rules. Otherwise looks good to me! 🙂
val nextWidth = indentWidth(next.offset) | ||
val lastWidth = currentRegion.indentWidth | ||
if lastWidth < nextWidth then | ||
currentRegion = Indented(nextWidth, COLONeol, currentRegion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it expected to use COLONeol
here? Or could use ARROW
instead?
At first sight, it only seems to be used here where it's compared to MATCH
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first attempt used ARROW and used it during newline handling, but that was not needed. I changed to colon for uniformity only; the result here is only produced and discarded in lookahead scanner. Actually on my WIP branch, I used a new token ARROWeol
for when it is not a proper indent, as after MATCH
. (Region prefix is used
|| nextWidth == lastWidth && (indentPrefix == MATCH || indentPrefix == CATCH) && token != CASE then |
There is experimental (dubious) use of the prefix at https://github.com/scala/scala3/pull/22530/files just to show that the prefix, if observed, could have semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's get that merged then!
@@ -680,7 +693,6 @@ object Scanners { | |||
currentRegion = Indented(nextWidth, COLONeol, currentRegion) | |||
offset = next.offset | |||
token = INDENT | |||
end observeIndented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I also don't like these end markers 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the method used to be longer and had a blank line. I must have deleted it during other edits while experimenting, but here it's no longer needed.
I ought to have squashed. |
thank you both! |
Fixes #22193
The lookahead scanner used by
followingIsLambdaAfterColon
would recalculate the indentation width of its top region from the current offset, which is the colon token. That would cause detection of indentation to fail on the next line. This commit preserves the indentation width for that purpose. Subsequent parsing of the function literal proceeds normally.