Untyped error checks in catch statement #2055
Conversation
|
Don't forget to add your entry to the change log :-) |
Codecov Report
@@ Coverage Diff @@
## master #2055 +/- ##
==========================================
+ Coverage 89.68% 89.71% +0.02%
==========================================
Files 259 260 +1
Lines 15020 15039 +19
Branches 977 978 +1
==========================================
+ Hits 13471 13492 +21
+ Misses 1532 1530 -2
Partials 17 17
Continue to review full report at Codecov.
|
dirtydanee
force-pushed
the
untype-error-in-catch
branch
from
094c9b5
to
f136dd7
Compare
| import Foundation | ||
| import SourceKittenFramework | ||
|
|
||
| public struct UntypedErrorRule: ASTRule, OptInRule, ConfigurationProviderRule { |
There was a problem hiding this comment.
What do you think about changing it to UntypedErrorInCatchRule
There was a problem hiding this comment.
And then chaning the rule identifier and name as well
There was a problem hiding this comment.
Updated the naming as you suggested, its for sure more clear what does it.
| description: "Catch statements should not declare error variables without type casting", | ||
| kind: .lint, | ||
| nonTriggeringExamples: [ | ||
| "do {\n try foo() \n} catch {\n}", |
There was a problem hiding this comment.
can you format he entries better? Check how they're rendered in Rules.md. I think just improving the indentation of try foo() would already help.
There was a problem hiding this comment.
Fixed the indentation and removed the \n from the catch statements scope.
| private static let regularExpression = regex( | ||
| "catch" + // The catch keyword | ||
| "(?:" + // Start of the first non-capturing group | ||
| "\\s+" + // At least one any type of whitespace character |
There was a problem hiding this comment.
A whitespace is not required if parentheses are used: catch(let error) {.
Also, the rule currently wouldn't catch when they're used even with spaces.
There was a problem hiding this comment.
Updated the pattern to capture the errors with parentheses with and without spaces.
| } | ||
|
|
||
| return file.lines | ||
| .lazy |
There was a problem hiding this comment.
Does this really improve performance? I think lines will be calculated anyways.
| return file.lines | ||
| .lazy | ||
| .filter { $0.byteRange.contains(offset) } | ||
| .reduce([]) { _, line in |
There was a problem hiding this comment.
wouldn't it be more semantic to use flatMap here?
| public static let description = RuleDescription( | ||
| identifier: "untyped_error", | ||
| name: "Untyped error rule", | ||
| description: "Catch statements should not declare error variables without type casting", |
There was a problem hiding this comment.
Can you add a . in the end?
| identifier: "untyped_error", | ||
| name: "Untyped error rule", | ||
| description: "Catch statements should not declare error variables without type casting", | ||
| kind: .lint, |
There was a problem hiding this comment.
can you change to .idiomatic? .lint is more for validating things that could indicate a programmer's error.
| nonTriggeringExamples: [ | ||
| "do {\n try foo() \n} catch {\n}", | ||
| "do {\n try foo() \n} catch Error.invalidOperation {\n} catch {\n}", | ||
| "do {\n try foo() \n} catch let error as? MyError {\n} catch {\n}", |
There was a problem hiding this comment.
this is not a major issue, but I think catch let error as MyError should be used. I got a warning while compiling with the optional cast:
warning: cast from '_' to unrelated type 'MyError' always fails
| return StyleViolation(ruleDescription: type(of: self).description, | ||
| severity: configuration.severity, | ||
| location: location, | ||
| reason: configuration.consoleDescription) |
There was a problem hiding this comment.
Is there a reason why you're iterating through lines?
I have the impression that it'd be faster to make this a regular rule (not ASTRule) and just match the regex on the whole file, validating some syntax kinds (e.g. that catch is a keyword, as well as let/var).
There was a problem hiding this comment.
@marcelofabri Thanks for the review! This suggestion was a really spot on. I have decided to use ASTRule to be able to solve the issue with the commented out code. However the syntax kind validation works great, simplified the code by tons and should be faster too.
dirtydanee
force-pushed
the
untype-error-in-catch
branch
2 times, most recently
from
b2b5b1f
to
1fb429d
Compare
dirtydanee
force-pushed
the
untype-error-in-catch
branch
from
7e2aace
to
c67937b
Compare
|
@marcelofabri thoughts on the update? |
|
@dirtydanee Merged in #2101 🎉 |
Implements #2045