[Sema] Warn for redundant access-level modifiers on setters or used in an extension. #18623

dingobye · 2018-08-10T04:48:56Z

~~This patch adds warning for redundant access-level modifiers used in an extension.~~

This patch adds warnings for redundant access-level modifiers for two aspects: (1) used on members declared in an extension, and (2) used on property setters.

(1) For the following example, it produces a warning - 'public' modifier is redundant for instance method declared in a public extension.

 public extension SomeClass {
     public func foo() {}
+    ^~~~~~~
 }

(2) For instance below, it produces a warning - 'public(set)' modifier is redundant for a public property.

 public SomeClass {
     public public(set) var x: Int = 0
+           ^~~~~~~~~~~~
 }

In addition, it refines the diagnostics of access_control_ext_member_more issues, in case the fixit
could suggest redundant modifiers. Consider the example below, the existing fixit suggests replacing public by internal, which is indeed a redundant modifier. This patch changes the fixit from replacement to removal.

 internal extension SomeClass {
     public func foo() {}
     ^~~~~~
-    internal
 }

Moreover, the corresponding redundancy warnings in the existing code of Swift projects are also fixed. These include:

stdlib in the main Swift repo as fixed by this patch
swift-corelibs-foundation repo #1663
swift-corelibs-libdispatch repo #390
swift-corelibs-xctest repo #218
swiftpm repo #1746

Resolves SR-8453.

…sion. This patch adds warning for redundant access-level modifiers used in an extension. It also refines the diagnostics of access_control_ext_member_more issues, in case the fixit could suggest redundant modifiers. Resolves: SR-8453.

dingobye · 2018-08-10T04:49:37Z

@jrose-apple Hi Jordan, would you help review this, please?

jrose-apple

Thanks for picking this up, Ding! I have a few comments but the general approach seems reasonable.

jrose-apple · 2018-08-10T15:42:58Z

test/SILGen/accessibility_warnings.swift

-  public func extMemberPublic() {}
-  // CHECK-DAG: sil private @$S22accessibility_warnings12PublicStructV07extImplC033_5D2F2E026754A901C0FF90C404896D02LLyyF
-  private func extImplPublic() {}
-}


Please don't remove this; just add the expected diagnostics. It's still checking the SIL-level linkage.

jrose-apple · 2018-08-10T15:44:12Z

test/Constraints/tuple_arguments.swift

  public func apply<Result>(_ transform: ((Wrapped) -> Result)?) -> Result? {
    return self.flatMap { value in
      transform.map { $0(value) }
    }
  }

+  // expected-warning@+1 {{'public' modifier is redundant for instance method declared in a public extension}}


Since this test isn't really about the access modifiers, it's probably better to just remove the public from the extension (both here and in test/Compatibility).

jrose-apple · 2018-08-10T15:44:47Z

include/swift/AST/DiagnosticsSema.def

@@ -1189,7 +1189,12 @@ ERROR(access_control_setter_more,none,
      (AccessLevel, unsigned, AccessLevel))
 WARNING(access_control_ext_member_more,none,
    "declaring %select{%error|a fileprivate|an internal|a public|open}0 %1 in "
-    "%select{a private|a fileprivate|an internal|public|%error}2 extension",
+    "%select{a private|a fileprivate|an internal|a public|%error}2 extension",


Thanks for fixing this. :-)

jrose-apple · 2018-08-10T15:46:06Z

lib/Sema/MiscDiagnostics.cpp

-      diag.fixItReplace(attr->getLocation(), fixItString.drop_back());
+      if (shouldNotReplace) {
+        // Remove the attribute if replacement is not preferred.
+        diag.fixItRemove(attr->getRange());


Can you add a test with public(set)?

Good catch! I extended the solution to also produce warnings for redundant access modifiers on setters.

jrose-apple · 2018-08-10T15:46:57Z

lib/Sema/MiscDiagnostics.h

+                 ValueDecl *VD,
+                 AccessLevel desiredAccess,
+                 bool isForSetter = false,
+                 bool shouldNotReplace = false);


Nitpick: "shouldNotReplace" doesn't tell us what to do instead. Maybe "shouldUseDefaultAccess" or something?

Yeah, shouldUseDefaultAccess sounds better!

dingobye · 2018-08-11T10:56:08Z

Thank you for your feedback, Jordan! I updated the code as you suggested.

In addition, this update produces a warning for the following case:

'fileprivate' modifier is redundant for instance method declared in a private (equivalent to fileprivate) extension

 private extension SomeClass {
     fileprivate func foo() {}
+    ^~~~~~~~~~~~
 }

My earlier solution didn't report any warning for such cases. I think it more appropriate to warn, since it makes the entire redundancy checking comprehensive and consistent.

modifier is used in an extension. In addition, add warnings for access modifier redundancy on property setters; and address comments from Jordan Rose.

jrose-apple · 2018-08-13T21:26:59Z

I think that's consistent, yes. We warn if the access is greater and if the access is the same, so it makes sense to also handle "written greater but effectively the same".

jrose-apple · 2018-08-13T22:10:42Z

@swift-ci Please test

dingobye · 2018-08-14T00:50:10Z

Thank you for the reply, Jordan!

I had stdlib compiled with this patch, and got flooded by a number of such warnings for redundant modifiers in extension. Below is the brief statistics, among which 588 are public, 2 are internal, and the rest 2 are fileprivate.

Source	Count
public/SDK/AppKit	16
public/SDK/CloudKit	2
public/SDK/CoreGraphics	4
public/SDK/Dispatch	202
public/SDK/Foundation	230
public/SDK/Intents	4
public/SDK/XCTest	4
public/core	130
TOTAL	592

Should all those warnings be fixed before landing this PR?

jrose-apple · 2018-08-14T01:06:14Z

Ooh, good thing to check. I should have looked at that first. Yes, we should definitely fix all those, but I can see Dispatch, Foundation, and XCTest all having different preferences from the rest (as the three teams who manage corelibs equivalents for their libraries). @mwwa, @phausler / @millenomi, and @briancroom, would you prefer to standardize on

public extension X {
  func foo()
}

or

extension X {
  public func foo()
}

for your respective projects?

phausler · 2018-08-14T03:01:13Z

Personally I prefer the latter because it enforces knowingly making things public

dingobye · 2018-08-15T07:27:09Z

Thank you, Philippe! I updated the code to fix the Foundation part as per your preference. Let's wait for advice from @mwwa and @briancroom.

mwwa · 2018-08-15T17:46:45Z

I'm also happy with @phausler's preference, seems reasonable. We'll need to perform the equivalent fixes in corelibs-dispatch too.

adapted to swiftlang/swift#18623.

dingobye · 2018-08-16T04:24:24Z

Thanks for your inputs, Matt! I updated the code for the Dispatch part, and submitted a PR for relevant fixes in the corelibs-libdispatch repo.

adapted to swiftlang/swift#18623.

dingobye · 2018-09-04T23:07:53Z

@jrose-apple Hi Jordan, thanks for your all time support, and now all relevant projects have been prepared to embrace this patch. Would you help finalise this, please?

jrose-apple · 2018-09-04T23:17:45Z

Ah, I thought we were still waiting on XCTest. I missed that Stuart already took care of that.

@swift-ci Please test

dingobye · 2018-09-04T23:23:17Z

Yes, Stuart helped XCTest earlier.

The last one we were waiting for was libdispatch, which was merged just hours ago. Thanks to Kim!

swift-ci · 2018-09-05T00:30:36Z

Build failed
Swift Test Linux Platform
Git Sha - d5d17fa

jrose-apple · 2018-09-05T01:30:43Z

@swift-ci Please test Linux

jrose-apple · 2018-09-06T00:44:44Z

Thanks for all your work here!

…d to swiftlang/swift#18623. Signed-off-by: Kim Topley <[email protected]>

jrose-apple reviewed Aug 10, 2018

View reviewed changes

dingobye changed the title ~~[Sema] Warn when redundant access-level modifier is added in an extension~~ [Sema] Warn for redundant access-level modifiers on setters or used in an extension. Aug 11, 2018

dingobye force-pushed the sr8453 branch from 4054a13 to 20b60b9 Compare August 11, 2018 11:18

Make it more comprehensive to warn when redundant access

63e1937

modifier is used in an extension. In addition, add warnings for access modifier redundancy on property setters; and address comments from Jordan Rose.

dingobye force-pushed the sr8453 branch from 20b60b9 to 63e1937 Compare August 11, 2018 11:24

jrose-apple self-assigned this Aug 12, 2018

jrose-apple approved these changes Aug 13, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

Fix warnings for stdlib/public/SDK/Foundation

2c00516

Fix warnings for stdlib/public/SDK/Dispatch

ea21794

dingobye pushed a commit to dingobye/swift-corelibs-foundation that referenced this pull request Aug 16, 2018

Fix redundant access-level modifiers to make corelibs-foundation

0c2fe5c

adapted to swiftlang/swift#18623.

dingobye pushed a commit to dingobye/swift-corelibs-libdispatch that referenced this pull request Aug 16, 2018

Fix redundant access-level modifiers to make corelibs-dispatch

4322cd1

adapted to swiftlang/swift#18623.

dingobye added a commit to dingobye/swift-corelibs-foundation that referenced this pull request Aug 16, 2018

Fix redundant access-level modifiers to make corelibs-foundation

fc386e0

adapted to swiftlang/swift#18623.

dingobye added a commit to dingobye/swift-corelibs-libdispatch that referenced this pull request Aug 16, 2018

Fix redundant access-level modifiers to make corelibs-dispatch

6391d76

adapted to swiftlang/swift#18623.

dingobye mentioned this pull request Aug 16, 2018

Fix redundant access-level modifiers. swiftlang/swift-corelibs-libdispatch#390

Merged

dingobye mentioned this pull request Aug 16, 2018

Fix redundant access-level modifiers. swiftlang/swift-corelibs-foundation#1663

Merged

dingobye added a commit to dingobye/swift-package-manager that referenced this pull request Aug 16, 2018

Fix redundant access-level modifiers to make swiftpm

48e8009

adapted to swiftlang/swift#18623.

dingobye mentioned this pull request Aug 16, 2018

Fix redundant access-level modifiers. swiftlang/swift-package-manager#1746

Merged

Fix warnings for stdlib/public/SDK/XCTest

a0ebe3a

dingobye added a commit to dingobye/swift-corelibs-xctest that referenced this pull request Aug 16, 2018

Fix redundant access-level modifiers to make corelibs-xctest

ae3d921

adapted to swiftlang/swift#18623.

dingobye mentioned this pull request Aug 16, 2018

Fix redundant access-level modifiers. swiftlang/swift-corelibs-xctest#218

Merged

Fix warnings for the rest of stdlib

d5d17fa

aciidgh pushed a commit to swiftlang/swift-package-manager that referenced this pull request Aug 16, 2018

Fix redundant access-level modifiers to make swiftpm

842f95d

adapted to swiftlang/swift#18623.

sindresorhus mentioned this pull request Aug 19, 2018

Rule Request: Place access modifiers on extension methods and not on the extension itself realm/SwiftLint#2356

Closed

2 tasks

parkera pushed a commit to swiftlang/swift-corelibs-foundation that referenced this pull request Aug 28, 2018

Fix redundant access-level modifiers to make corelibs-foundation (#1663)

5b3dae9

adapted to swiftlang/swift#18623.

jrose-apple merged commit 5dace22 into swiftlang:master Sep 6, 2018

ktopley-apple pushed a commit to swiftlang/swift-corelibs-libdispatch that referenced this pull request Dec 6, 2018

Fix redundant access-level modifiers to make corelibs-dispatch adapte…

de5cc6f

…d to swiftlang/swift#18623. Signed-off-by: Kim Topley <[email protected]>

[Sema] Warn for redundant access-level modifiers on setters or used in an extension. #18623

[Sema] Warn for redundant access-level modifiers on setters or used in an extension. #18623

Uh oh!

Conversation

dingobye commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dingobye commented Aug 10, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingobye commented Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Aug 13, 2018

Uh oh!

jrose-apple commented Aug 13, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

dingobye commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Aug 14, 2018

Uh oh!

phausler commented Aug 14, 2018

Uh oh!

dingobye commented Aug 15, 2018

Uh oh!

mwwa commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dingobye commented Aug 16, 2018

Uh oh!

dingobye commented Sep 4, 2018

Uh oh!

jrose-apple commented Sep 4, 2018

Uh oh!

dingobye commented Sep 4, 2018

Uh oh!

swift-ci commented Sep 5, 2018

Uh oh!

jrose-apple commented Sep 5, 2018

Uh oh!

jrose-apple commented Sep 6, 2018

Uh oh!

Uh oh!

dingobye commented Aug 10, 2018 •

edited

Loading

dingobye commented Aug 11, 2018 •

edited

Loading

dingobye commented Aug 14, 2018 •

edited

Loading

mwwa commented Aug 15, 2018 •

edited

Loading