Skip to content

Warn on misleading indentation in single-case catch #14738

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 23, 2022
Merged

Warn on misleading indentation in single-case catch #14738

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2855693
Warn on misleading indentation in single-case catch
odersky Mar 21, 2022
36300dc
Fix ShadowingTest try catch indentation
odersky Mar 21, 2022
d7fb8bf
Fix fixes in backend and improve warning message
odersky Mar 21, 2022
a670518
Change interpretation of ShadowingTests again
odersky Mar 21, 2022
a7ffc8e
Update tests/neg-custom-args/fatal-warnings/i14721.scala
odersky Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/backend/jvm/BytecodeWriters.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ trait BytecodeWriters {
catch case ex: ClosedByInterruptException =>
try
outfile.delete() // don't leave an empty or half-written classfile around after an interrupt
catch case _: Throwable =>
catch
case _: Throwable =>
throw ex
finally outstream.close()
report.informProgress("wrote '" + label + "' to " + outfile)
Expand Down
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/backend/jvm/GenBCode.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ class GenBCodePipeline(val int: DottyBackendInterface, val primitives: DottyPrim
catch case ex: ClosedByInterruptException =>
try
outTastyFile.delete() // don't leave an empty or half-written tastyfile around after an interrupt
catch case _: Throwable =>
catch
case _: Throwable =>
throw ex
finally outstream.close()

Expand Down
9 changes: 8 additions & 1 deletion compiler/src/dotty/tools/dotc/parsing/Parsers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2620,7 +2620,14 @@ object Parsers {
(pattern(), guard())
}
CaseDef(pat, grd, atSpan(accept(ARROW)) {
if exprOnly then expr() else block()
if exprOnly then
if in.indentSyntax && in.isAfterLineEnd && in.token != INDENT then
warning(i"""Misleading indentation: this expression forms part of the preceding catch case.
|If this is intended, it should be indented for clarity.
|Otherwise, if the handler is intended to be empty, use a multi-line catch with
|an indented case.""")
expr()
else block()
})
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/test/dotty/tools/repl/ShadowingTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ object ShadowingTests:
val subdir = dir.resolve(name)
try Files.createDirectory(subdir)
catch case _: java.nio.file.FileAlreadyExistsException =>
assert(Files.isDirectory(subdir), s"failed to create shadowed subdirectory $subdir")
assert(Files.isDirectory(subdir), s"failed to create shadowed subdirectory $subdir")
Comment on lines 35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, I believe my intention there was to swallow the FileAlreadyExistsException, not for the assert to be the body of the catch ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fixed. Goes to show that this really is a dangerous trap.

Can we make it an error? I don't see how without undermining fundamental assumptions how the parser works. According to the syntax we need an Expr and that's what we can parse. Rejecting it would mean we throw aspects which are outside the context-free syntax into the parsing algorithm.

Copy link
Contributor

Choose a reason for hiding this comment

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

No wait, it's asserting that the existing thing is a dir, right? If createDir succeeds, it better be a dir.

Copy link
Contributor

Choose a reason for hiding this comment

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

One way to clarify the syntax is to require the expr to begin on the same line as the arrow. So the arrow could move to the following line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact the warning occurrences in the backend were also assuming an empty
catch handler. I changed them to do the right thing, and fixed the warning message
to point out this possibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@som-snytt I think you are right for ShadowingTests. The assert really is meant as a handler. @griggt can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm almost certain that when I wrote it I meant for the catch handler to be empty. But in this case it works either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, we need more forms of syntax that work either way. If Booleans worked either way, we'd never take the wrong branch.

subdir

// The directory on the classpath containing artifacts to be shadowed
Expand Down
9 changes: 9 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i14721.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import scala.util.chaining.given
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be useless/irrelevant.


class C:
def op: Unit = println("op")
def handler: Unit = println("handler")
def test: Unit =
try op
catch case _: NullPointerException =>
handler // error