Skip to content

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

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

Merged
merged 6 commits into from
Sep 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -1187,9 +1187,19 @@ ERROR(access_control_setter_more,none,
"%select{variable|property|subscript}1 cannot have "
"%select{%error|a fileprivate|an internal|a public|an open}2 setter",
(AccessLevel, unsigned, AccessLevel))
WARNING(access_control_setter_redundant,none,
"'%select{private|fileprivate|internal|public|open}0(set)' modifier is "
"redundant for %select{a private|a fileprivate|an internal|a public|an open}2 "
"%1",
(AccessLevel, DescriptiveDeclKind, 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",
(AccessLevel, DescriptiveDeclKind, AccessLevel))
WARNING(access_control_ext_member_redundant,none,
"'%select{%error|fileprivate|internal|public|%error}0' modifier is redundant "
"for %1 declared in %select{a private (equivalent to fileprivate)|a fileprivate"
"|an internal|a public|%error}2 extension",
(AccessLevel, DescriptiveDeclKind, AccessLevel))
ERROR(access_control_ext_requirement_member_more,none,
"cannot declare %select{%error|a fileprivate|an internal|a public|an open}0 %1 "
Expand Down
14 changes: 10 additions & 4 deletions lib/Sema/MiscDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3951,7 +3951,8 @@ void swift::performStmtDiagnostics(TypeChecker &TC, const Stmt *S) {
//===----------------------------------------------------------------------===//

void swift::fixItAccess(InFlightDiagnostic &diag, ValueDecl *VD,
AccessLevel desiredAccess, bool isForSetter) {
AccessLevel desiredAccess, bool isForSetter,
bool shouldUseDefaultAccess) {
StringRef fixItString;
switch (desiredAccess) {
case AccessLevel::Private: fixItString = "private "; break;
Expand Down Expand Up @@ -3997,9 +3998,14 @@ void swift::fixItAccess(InFlightDiagnostic &diag, ValueDecl *VD,
// this function is sometimes called to make access narrower, so assuming
// that a broader scope is acceptable breaks some diagnostics.
if (attr->getAccess() != desiredAccess) {
// This uses getLocation() instead of getRange() because we don't want to
// replace the "(set)" part of a setter attribute.
diag.fixItReplace(attr->getLocation(), fixItString.drop_back());
if (shouldUseDefaultAccess) {
// Remove the attribute if replacement is not preferred.
diag.fixItRemove(attr->getRange());
} else {
// This uses getLocation() instead of getRange() because we don't want to
// replace the "(set)" part of a setter attribute.
diag.fixItReplace(attr->getLocation(), fixItString.drop_back());
}
attr->setInvalid();
}

Expand Down
7 changes: 5 additions & 2 deletions lib/Sema/MiscDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,11 @@ void performTopLevelDeclDiagnostics(TypeChecker &TC, TopLevelCodeDecl *TLCD);
/// Emit a fix-it to set the access of \p VD to \p desiredAccess.
///
/// This actually updates \p VD as well.
void fixItAccess(InFlightDiagnostic &diag, ValueDecl *VD,
AccessLevel desiredAccess, bool isForSetter = false);
void fixItAccess(InFlightDiagnostic &diag,
ValueDecl *VD,
AccessLevel desiredAccess,
bool isForSetter = false,
bool shouldUseDefaultAccess = false);

/// Emit fix-its to correct the argument labels in \p expr, which is the
/// argument tuple or single argument of a call.
Expand Down
27 changes: 21 additions & 6 deletions lib/Sema/TypeCheckAttr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1512,17 +1512,23 @@ void AttributeChecker::visitAccessControlAttr(AccessControlAttr *attr) {

if (auto extAttr =
extension->getAttrs().getAttribute<AccessControlAttr>()) {
// Extensions are top level declarations, for which the literally lowest
// access level `private` is equivalent to `fileprivate`.
AccessLevel extAccess = std::max(extAttr->getAccess(),
AccessLevel::FilePrivate);
if (attr->getAccess() > extAccess) {
AccessLevel defaultAccess = extension->getDefaultAccessLevel();
if (attr->getAccess() > defaultAccess) {
auto diag = TC.diagnose(attr->getLocation(),
diag::access_control_ext_member_more,
attr->getAccess(),
D->getDescriptiveKind(),
extAttr->getAccess());
swift::fixItAccess(diag, cast<ValueDecl>(D), extAccess);
swift::fixItAccess(diag, cast<ValueDecl>(D), defaultAccess, false,
true);
return;
} else if (attr->getAccess() == defaultAccess) {
TC.diagnose(attr->getLocation(),
diag::access_control_ext_member_redundant,
attr->getAccess(),
D->getDescriptiveKind(),
extAttr->getAccess())
.fixItRemove(attr->getRange());
return;
}
}
Expand Down Expand Up @@ -1558,6 +1564,15 @@ AttributeChecker::visitSetterAccessAttr(SetterAccessAttr *attr) {
getterAccess, storageKind, attr->getAccess());
attr->setInvalid();
return;

} else if (attr->getAccess() == getterAccess) {
TC.diagnose(attr->getLocation(),
diag::access_control_setter_redundant,
attr->getAccess(),
D->getDescriptiveKind(),
getterAccess)
.fixItRemove(attr->getRange());
return;
}
}

Expand Down
4 changes: 2 additions & 2 deletions stdlib/public/SDK/AppKit/AppKit_FoundationExtensions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import Foundation
@_exported import AppKit

// NSCollectionView extensions
public extension IndexPath {
extension IndexPath {

/// Initialize for use with `NSCollectionView`.
public init(item: Int, section: Int) {
Expand Down Expand Up @@ -51,7 +51,7 @@ public extension IndexPath {

}

public extension URLResourceValues {
extension URLResourceValues {
/// Returns all thumbnails as a single NSImage.
@available(macOS 10.10, *)
public var thumbnail : NSImage? {
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/SDK/CloudKit/NestedNames.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

@nonobjc
@available(macOS 10.10, iOS 8.0, watchOS 3.0, *)
public extension CKContainer {
extension CKContainer {
@available(swift 4.2)
public enum Application {
public typealias Permissions = CKContainer_Application_Permissions
Expand Down
4 changes: 2 additions & 2 deletions stdlib/public/SDK/CoreGraphics/CoreGraphics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -429,10 +429,10 @@ public extension CGRect {
}

@available(*, unavailable, renamed: "minX")
public var x: CGFloat { return minX }
var x: CGFloat { return minX }

@available(*, unavailable, renamed: "minY")
public var y: CGFloat { return minY }
var y: CGFloat { return minY }
}

extension CGRect : CustomReflectable {
Expand Down
4 changes: 2 additions & 2 deletions stdlib/public/SDK/Dispatch/Dispatch.swift
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ public enum DispatchTimeoutResult {

/// dispatch_group

public extension DispatchGroup {
extension DispatchGroup {
public func notify(qos: DispatchQoS = .unspecified, flags: DispatchWorkItemFlags = [], queue: DispatchQueue, execute work: @escaping @convention(block) () -> Void) {
if #available(macOS 10.10, iOS 8.0, *), qos != .unspecified || !flags.isEmpty {
let item = DispatchWorkItem(qos: qos, flags: flags, block: work)
Expand Down Expand Up @@ -159,7 +159,7 @@ public extension DispatchGroup {

/// dispatch_semaphore

public extension DispatchSemaphore {
extension DispatchSemaphore {
@discardableResult
public func signal() -> Int {
return __dispatch_semaphore_signal(self)
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/SDK/Dispatch/IO.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//
//===----------------------------------------------------------------------===//

public extension DispatchIO {
extension DispatchIO {

public enum StreamType : UInt {
case stream = 0
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/SDK/Dispatch/Queue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ internal class _DispatchSpecificValue<T> {
internal init(value: T) { self.value = value }
}

public extension DispatchQueue {
extension DispatchQueue {
public struct Attributes : OptionSet {
public let rawValue: UInt64
public init(rawValue: UInt64) { self.rawValue = rawValue }
Expand Down
24 changes: 12 additions & 12 deletions stdlib/public/SDK/Dispatch/Source.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
// import Foundation
import _SwiftDispatchOverlayShims

public extension DispatchSourceProtocol {
typealias DispatchSourceHandler = @convention(block) () -> Void
extension DispatchSourceProtocol {
public typealias DispatchSourceHandler = @convention(block) () -> Void

public func setEventHandler(qos: DispatchQoS = .unspecified, flags: DispatchWorkItemFlags = [], handler: DispatchSourceHandler?) {
if #available(macOS 10.10, iOS 8.0, *),
Expand Down Expand Up @@ -98,7 +98,7 @@ public extension DispatchSourceProtocol {
}
}

public extension DispatchSource {
extension DispatchSource {
public struct MachSendEvent : OptionSet, RawRepresentable {
public let rawValue: UInt
public init(rawValue: UInt) { self.rawValue = rawValue }
Expand Down Expand Up @@ -210,7 +210,7 @@ public extension DispatchSource {
}
}

public extension DispatchSourceMachSend {
extension DispatchSourceMachSend {
public var handle: mach_port_t {
return mach_port_t(__dispatch_source_get_handle(self as! DispatchSource))
}
Expand All @@ -226,13 +226,13 @@ public extension DispatchSourceMachSend {
}
}

public extension DispatchSourceMachReceive {
extension DispatchSourceMachReceive {
public var handle: mach_port_t {
return mach_port_t(__dispatch_source_get_handle(self as! DispatchSource))
}
}

public extension DispatchSourceMemoryPressure {
extension DispatchSourceMemoryPressure {
public var data: DispatchSource.MemoryPressureEvent {
let data = __dispatch_source_get_data(self as! DispatchSource)
return DispatchSource.MemoryPressureEvent(rawValue: data)
Expand All @@ -244,7 +244,7 @@ public extension DispatchSourceMemoryPressure {
}
}

public extension DispatchSourceProcess {
extension DispatchSourceProcess {
public var handle: pid_t {
return pid_t(__dispatch_source_get_handle(self as! DispatchSource))
}
Expand All @@ -260,7 +260,7 @@ public extension DispatchSourceProcess {
}
}

public extension DispatchSourceTimer {
extension DispatchSourceTimer {
///
/// Sets the deadline and leeway for a timer event that fires once.
///
Expand Down Expand Up @@ -599,7 +599,7 @@ public extension DispatchSourceTimer {
}
}

public extension DispatchSourceFileSystemObject {
extension DispatchSourceFileSystemObject {
public var handle: Int32 {
return Int32(__dispatch_source_get_handle(self as! DispatchSource))
}
Expand All @@ -615,7 +615,7 @@ public extension DispatchSourceFileSystemObject {
}
}

public extension DispatchSourceUserDataAdd {
extension DispatchSourceUserDataAdd {
/// @function add
///
/// @abstract
Expand All @@ -630,7 +630,7 @@ public extension DispatchSourceUserDataAdd {
}
}

public extension DispatchSourceUserDataOr {
extension DispatchSourceUserDataOr {
/// @function or
///
/// @abstract
Expand All @@ -645,7 +645,7 @@ public extension DispatchSourceUserDataOr {
}
}

public extension DispatchSourceUserDataReplace {
extension DispatchSourceUserDataReplace {
/// @function replace
///
/// @abstract
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/SDK/Foundation/Codable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ extension DecodingError : LocalizedError {}
// Error Utilities
//===----------------------------------------------------------------------===//

internal extension DecodingError {
extension DecodingError {
/// Returns a `.typeMismatch` error describing the expected type.
///
/// - parameter path: The path of `CodingKey`s taken to decode a value of this type.
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/SDK/Foundation/JSONEncoder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2432,7 +2432,7 @@ fileprivate var _iso8601Formatter: ISO8601DateFormatter = {
// Error Utilities
//===----------------------------------------------------------------------===//

fileprivate extension EncodingError {
extension EncodingError {
/// Returns a `.invalidValue` error describing the given invalid floating-point value.
///
///
Expand Down
10 changes: 5 additions & 5 deletions stdlib/public/SDK/Foundation/NSError.swift
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ extension _BridgedStoredNSError {
}

/// Implementation of _ObjectiveCBridgeableError for all _BridgedStoredNSErrors.
public extension _BridgedStoredNSError {
extension _BridgedStoredNSError {
/// Default implementation of ``init(_bridgedNSError:)`` to provide
/// bridging from NSError.
public init?(_bridgedNSError error: NSError) {
Expand Down Expand Up @@ -617,7 +617,7 @@ public extension CocoaError {
}
}

public extension CocoaError {
extension CocoaError {
public static func error(_ code: CocoaError.Code, userInfo: [AnyHashable : Any]? = nil, url: URL? = nil) -> Error {
var info: [AnyHashable : Any] = userInfo ?? [:]
if let url = url {
Expand Down Expand Up @@ -1821,7 +1821,7 @@ public struct URLError : _BridgedStoredNSError {
}
}

public extension URLError.Code {
extension URLError.Code {
public static var unknown: URLError.Code {
return URLError.Code(rawValue: -1)
}
Expand Down Expand Up @@ -1984,7 +1984,7 @@ public extension URLError.Code {
}
}

public extension URLError {
extension URLError {
private var _nsUserInfo: [AnyHashable : Any] {
return (self as NSError).userInfo
}
Expand All @@ -2009,7 +2009,7 @@ public extension URLError {
}
}

public extension URLError {
extension URLError {
public static var unknown: URLError.Code {
return .unknown
}
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/SDK/Foundation/NSObject.swift
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ public class NSKeyValueObservation : NSObject {
}
}

public extension _KeyValueCodingAndObserving {
extension _KeyValueCodingAndObserving {

///when the returned NSKeyValueObservation is deinited or invalidated, it will stop observing
public func observe<Value>(
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/SDK/Foundation/Progress.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

@_exported import Foundation // Clang module

public extension Progress {
extension Progress {
@available(macOS 10.13, iOS 11.0, watchOS 4.0, tvOS 11.0, *)
public var estimatedTimeRemaining: TimeInterval? {
get {
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/SDK/Intents/INIntent.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import Foundation

public protocol _INIntentSetImageKeyPath { }

public extension _INIntentSetImageKeyPath {
extension _INIntentSetImageKeyPath {

@available(iOS 12.0, watchOS 5.0, macOS 10.14, *)
public func setImage<Value>(_ image: INImage?, forParameterNamed parameterName: KeyPath<Self, Value>) {
Expand Down
4 changes: 2 additions & 2 deletions stdlib/public/SDK/XCTest/XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import _SwiftXCTestOverlayShims

// --- XCTest API Swiftification ---

public extension XCTContext {
extension XCTContext {

/// Create and run a new activity with provided name and block.
public class func runActivity<Result>(named name: String, block: (XCTActivity) throws -> Result) rethrows -> Result {
Expand All @@ -40,7 +40,7 @@ public extension XCTContext {
#if os(macOS)
@available(swift 4.0)
@available(macOS 10.11, *)
public extension XCUIElement {
extension XCUIElement {
/// Types a single key from the XCUIKeyboardKey enumeration with the specified modifier flags.
@nonobjc public func typeKey(_ key: XCUIKeyboardKey, modifierFlags: XCUIElement.KeyModifierFlags) {
// Call the version of the method defined in XCTest.framework.
Expand Down
Loading