-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add LowerACLThanParentRule #2136
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @keith, please leave two trailing spaces after the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -98,6 +98,10 @@ class RulesTests: XCTestCase { | |||
verifyRule(EmptyParametersRule.description) | |||
} | |||
|
|||
func testLowerACLThanParent() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include the Linux tests as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
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