Skip to content
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

Check trailing blank line at EOF for OUTDENT #22855

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Mar 21, 2025

Boost the previous fix for "spurious indentation error for blank line at EOF" by checking that the line is a non-empty blank line.

This accommodates REPL, which routinely parses lines at EOF.

Also improve the previous fix for "correctly detect colon arrow at EOL" by returning token EOF at EOF. The check for EOF when probing whether followingIsLambaAfterColon was specifically to accommodate REPL.

Fixes #22844

@som-snytt som-snytt force-pushed the issue/22844-repl-eol branch from ab9caa0 to 1d740e9 Compare March 21, 2025 17:56
@som-snytt
Copy link
Contributor Author

An adjustment is required.

scala> println:
     |   42
     |
42

scala> List(42).map: x =>
     |   x + 1
     |
val res0: List[Int] = List(43)

scala> List(42).map: x =>
-- Error: ----------------------------------------------------------------------
1 |List(42).map: x =>
  |^^^^^^^^^^^^^^^
  |not a legal formal parameter for a function literal

scala>

where res0 was from history, and the error shows "arrow eol" is still broken.

Also the scripted test does not actually pass.

In incomplete state, REPL re-parses on every character (IIUC); I expected to follow up with other improvements in that area.

@som-snytt som-snytt force-pushed the issue/22844-repl-eol branch from 1d740e9 to eb2c406 Compare March 22, 2025 10:04
@som-snytt
Copy link
Contributor Author

Dotty ReplTest does only single-line inputs; at least "scripted" test should work like Scala 2 SessionTest.

The gold standard integration test would run a REPL process from test input, in order to test incomplete input:

List(42).map: x =>// at eof
List(42).map: x =>\n// at eof
List(42).map: x =>\n  x+1// at eof
List(42).map: x =>\n  x+1\n\n // complete

The JLine parser could be tested for internal behavior, but scripted or "session" tests are easier to create and read.

@som-snytt som-snytt force-pushed the issue/22844-repl-eol branch from eb2c406 to 475b235 Compare March 22, 2025 11:41
@som-snytt som-snytt marked this pull request as ready for review March 22, 2025 16:19
@Gedochao Gedochao requested review from mbovel and hamzaremmal March 24, 2025 06:54
@Gedochao Gedochao added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Mar 24, 2025
@SethTisue SethTisue added this to the 3.7.0 milestone Mar 24, 2025
@som-snytt som-snytt force-pushed the issue/22844-repl-eol branch from 475b235 to 477d593 Compare March 29, 2025 21:42
@som-snytt
Copy link
Contributor Author

The failure was the flaky cats test. Of course, dog people know that all cats are flaky.

@som-snytt som-snytt force-pushed the issue/22844-repl-eol branch from 477d593 to 70c7af2 Compare April 5, 2025 05:37
@tgodzik
Copy link
Contributor

tgodzik commented Apr 7, 2025

@mbovel @hamzaremmal Looks like this is ready to review, anyone could take a look?

@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

Yes, I can have a look tomorrow 👍

@mbovel
Copy link
Member

mbovel commented Apr 8, 2025

Let me try to summarize.

Consider the following snippet without a trailing line break (i.e., the last three tokens are NEWLINE, SPACE, and EOF):

object Foo:
••val foo = 42
  1. Following No outdent at eof #22435, no DEDENT token is emitted for the last line. That avoids emitting an indentation error, which is desirable since the user might be writing text on the last line and might not be finished yet (a space on the last line of the file causes a compilation error #22332). The test tests/pos/i22332.scala demonstrates that.

  2. However, we still want to emit a DEDENT token if the last line is completely empty (i.e., the last two tokens are NEWLINE followed by EOF), otherwise the REPL never considers the input complete (optional braces no longer work in repl #22844). This is what this PR achieves. The new test i22844 regression colon eol in dotty.tools.repl.ReplCompilerTests demonstrates that second situation.

Is my understanding correct? If yes, then the implementation looks correct to me! 🙂

@mbovel
Copy link
Member

mbovel commented Apr 8, 2025

Out of curiosity, I also checked what happens when a non-empty blank line is not at the end—for example, when the trailing four tokens are NEWLINE, SPACE, NEWLINE, and EOF, i.e., the last line is empty but the one before has a space. This also doesn't report an error. handleNewLine is not even entered for the line with the space in that case. This seems coherent 👍

Copy link
Member

@mbovel mbovel left a comment

Choose a reason for hiding this comment

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

Minor: I would not include 7fefe7f, a commit that only reformats code. The improvement looks subjective to me. The removal of removeNumberSeparators or the loss of indentation for cases in isIncomplete actually make the code harder to read for me. Such changes also pollute git blame.

@som-snytt
Copy link
Contributor Author

som-snytt commented Apr 8, 2025

Thanks for your review effort! I will also try to understand my code and clean it up.

I'm not wedded to the first commit, which I see as scouting. I think "match-aligned case" is the future, but the future doesn't have to be now.

Ah yes, to rephrase, the additional condition !isTrailingBlankLine says the last line must not match \s+ for outdent.

The change to observeArrowIndented says no indent at EOF for REPL.

@som-snytt som-snytt force-pushed the issue/22844-repl-eol branch from 70c7af2 to ce4d8de Compare April 8, 2025 16:29
@mbovel mbovel self-requested a review April 8, 2025 16:31
@som-snytt
Copy link
Contributor Author

posCC failed.

Check trailing blank line at EOF for OUTDENT

If EOF is preceded by only spaces on the last line, do not outdent
because it will complain about alignment during an edit.

Preserve EOF token when probing arrow EOL

For REPL, `: x =>` at EOF sees an EOF in order to detect lambda eol.
@som-snytt som-snytt force-pushed the issue/22844-repl-eol branch from ce4d8de to 268b325 Compare April 8, 2025 19:24
@WojciechMazur WojciechMazur added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Apr 8, 2025
@som-snytt som-snytt merged commit bc6cf47 into scala:main Apr 9, 2025
29 checks passed
@som-snytt som-snytt deleted the issue/22844-repl-eol branch April 9, 2025 00:07
WojciechMazur added a commit that referenced this pull request Apr 9, 2025
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optional braces no longer work in repl
6 participants