Add LowerACLThanParentRule #2136
Conversation
keith
force-pushed
the
ks/lower-acl
branch
2 times, most recently
from
90d0b16
to
141dabf
Compare
|
There are 2 cases where I think it could be argued that this is a downside. The first is the required to fix the bug I linked in the description, but if you have this valid definition: public protocol Foo {}
extension Foo {
public func bar() {}
}You would now have to annotate The other case is with using private struct Foo {
fileprivate var bar: String?
}But this would be required to change to These are some downsides, but overall I think this would still be worth it for us in particular! |
|
@keith Running the unit tests locally should fail on the first time, but update the docs. It shouldn't fail after Rules.md is updated. |
|
Updated! |
Codecov Report
@@ Coverage Diff @@
## master #2136 +/- ##
==========================================
+ Coverage 89.88% 89.92% +0.04%
==========================================
Files 266 267 +1
Lines 15375 15431 +56
Branches 1012 1022 +10
==========================================
+ Hits 13820 13877 +57
+ Misses 1535 1534 -1
Partials 20 20
Continue to review full report at Codecov.
|
CHANGELOG.md
Outdated
| @@ -6,6 +6,10 @@ | |||
|
|
|||
| #### Enhancements | |||
|
|
|||
| * Add `LowerACLThanParent` rule | |||
There was a problem hiding this comment.
Hi @keith, please leave two trailing spaces after the description.
| @@ -98,6 +98,10 @@ class RulesTests: XCTestCase { | |||
| verifyRule(EmptyParametersRule.description) | |||
| } | |||
|
|
|||
| func testLowerACLThanParent() { | |||
There was a problem hiding this comment.
Please include the Linux tests as well.
This new rule validates that if a type/function/variable definition has an ACL specifier, it is more restrictive than the containing body's level. This is intended to lint a peculiarity of SE-0025 where it is stated: > The compiler should not warn when a broader level of access control is used within a type with more restrictive access, such as internal within a private type. This allows the designer of the type to select the access they would use were they to make the type more widely accessible. I think this is an anti-goal because when a type is made more open, it should be a concious decision at that time to make it public. This is of course an opt-in rule as well. This also has the added benefit of linting this compiler bug: https://bugs.swift.org/browse/SR-2925
|
Given that this is opt-in, I'm comfortable with the false positives. Nicely implemented @keith! |
|
Thanks! |
|
Follow up to make this a bit more useful #2164 |
This new rule validates that if a type/function/variable definition has
an ACL specifier, it is more restrictive than the containing body's
level. This is intended to lint a peculiarity of SE-0025 where it is
stated:
I think this is an anti-goal because when a type is made more open, it
should be a concious decision at that time to make it public. This is of
course an opt-in rule as well. This also has the added benefit of
linting this compiler bug: https://bugs.swift.org/browse/SR-2925