Hello everyone! I am really excited to be a part of the Swift development community! I just made my first PR to swiftlang/swift that’s a fix for issue #43590.
In this PR, I’ve implemented a diagnostic in MiscDiagnostics.cpp that identifies empty catch blocks while specifically ignoring cases where the user has explicitly silenced the error, i.e., _ = error. I also included a Fix-it to suggest that idiomatic silencing. I'm a CS student and this is my first contribution to the compiler, so I'm eager to hear any feedback on the implementation!
Thanks in advance for the advice and Merry Christmas!!
I actually ran clang-format on the file because I saw a .clang-format in the root directory. Actual changes are pretty minor, line 6507 onwards. I noticed it and mentioned the same fact in the description too.
Yeah, found changes after posting the question. Just took quite a time for me to find, that’s why was asking, seems strange and a bit hard to to check PR. But I’m not a contributor, actually, was just interesting to check.
I actually did add the notice that clang-format made a lot of changes but it was at the bottom, I’ve since moved it up top to make it easier for anyone checking the PR out.
I concur, at the very least formatting changes belong to a separate commit if not a separate PR. But ideally they should only be applied to lines necessary for the fix.
Hi everyone! Thank you so much for helping me out on the first PR. I was really nervous and you all really came through to make feel like I’m a part. This means a lot to me.
Unfortunately, I’m on vacation and will be away from my machine for just a couple of days, so I will address all the issues in a few days time.
Thanks so much to everyone for the guidance, and I’m looking forward to learning more as we go!
Hi everyone! I just got back and have fixed all the issues pointed out by you all. Please check the PR out here again. Thanks for the guidance and lmk if there’s any modification needed!
after looking at the ticket, a couple thoughts occurred to me regarding the current implementation. the version in the PR at the moment will produce a warning for empty catch statements such as:
and allows silencing the diagnostic if the error is explicitly ignored like:
do {
try stuff()
} catch { _ = error }
my read of the ticket is that the 'spirit' of the proposed diagnostic would be to try and nudge people toward using try? in such cases if the error will simply be ignored. currently neither the proposed diagnostic text nor fixit appear to directly do that. also, i wonder if it wouldn't be a sufficient and slightly less burdensome silencing mechanism to suppress the diagnostic if the user writes:
do {
try stuff()
} catch _ {} // now we've indicated we intentionally don't care about handling
rather than having to ignore the implicit error variable.
Okay so I gave this some thought and I have decided upon a stricter approach:
Silencing using comments is not allowed (was implied in the ticket) and will be treated as an empty {}
The only valid method will be using catch _ {}
Any explicit silencing like catch let e { _ = e } or catch { _ = error} will now trigger a specific 'redundant' warning with a fix it pointing the user towards using catch _ {}.
And the empty {} warnings and fix its nudge the user towards using try? or to silence it as such catch _ {}
I am updating the implementation to reflect this now.
Just took quite a time for me to find, that’s why was asking, seems strange and a bit hard to to check PR. But I’m not a contributor, actually, was just interesting to check.