Skip to content

[Sema] Diagnose available attribute on wrapped and lazy properties #41112

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.

Sign up for GitHub

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 2 commits into from
Feb 7, 2022
Merged

[Sema] Diagnose available attribute on wrapped and lazy properties #41112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
093b8d3
[Sema] Emit an error for attempts to use `@available` on var declarat…
tshortli Jan 31, 2022
e34196d
Fix tests that depended on availability for lazy vars.
tshortli Feb 4, 2022
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
4 changes: 4 additions & 0 deletions include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5390,6 +5390,10 @@ class VarDecl : public AbstractStorageDecl {
/// an attached property wrapper.
VarDecl *getPropertyWrapperWrappedValueVar() const;

/// Return true if this property either has storage or has an attached property
/// wrapper that has storage.
bool hasStorageOrWrapsStorage() const;

/// Visit all auxiliary declarations to this VarDecl.
///
/// An auxiliary declaration is a declaration synthesized by the compiler to support
Expand Down
14 changes: 14 additions & 0 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6446,6 +6446,20 @@ VarDecl *VarDecl::getPropertyWrapperWrappedValueVar() const {
return getPropertyWrapperAuxiliaryVariables().localWrappedValueVar;
}

bool VarDecl::hasStorageOrWrapsStorage() const {
if (hasStorage())
return true;

if (getAttrs().hasAttribute<LazyAttr>())
return true;

auto *backing = getPropertyWrapperBackingProperty();
Copy link
Contributor

@slavapestov slavapestov Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a check for getAttributes().hasAttribute<LazyAttr>()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this suggestion is to avoid going into the request evaluator when it's not necessary. Should we instead add this check inside the VarDecl::getPropertyWrapper* methods to avoid doing it at all of the call sites?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, a formatting error there -- I meant to write getAttributes().hasAttribute<LazyAttr>() -- we need the same check for lazy properties as well.

I'm assuming the short-circuiting check you're thinking of is for CustomAttr. I remember this was worthwhile at one point because we weren't caching the results of some of these requests, but I'm not sure anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've added the check for the lazy attribute as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, yes, I thought you were talking about checking for custom attributes. Sorry!

if (backing && backing->hasStorage())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the backing property is always necessarily stored

return true;

return false;
}

void VarDecl::visitAuxiliaryDecls(llvm::function_ref<void(VarDecl *)> visit) const {
if (getDeclContext()->isTypeContext() || isImplicit())
return;
Expand Down
5 changes: 1 addition & 4 deletions lib/Sema/CodeSynthesis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,10 +928,7 @@ bool AreAllStoredPropertiesDefaultInitableRequest::evaluate(
if (VD->getAttrs().hasAttribute<NSManagedAttr>())
CheckDefaultInitializer = false;

if (VD->hasStorage())
HasStorage = true;
auto *backing = VD->getPropertyWrapperBackingProperty();
if (backing && backing->hasStorage())
if (VD->hasStorageOrWrapsStorage())
HasStorage = true;
});

Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/TypeCheckAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3562,7 +3562,7 @@ TypeChecker::diagnosticIfDeclCannotBePotentiallyUnavailable(const Decl *D) {
auto *DC = D->getDeclContext();

if (auto *VD = dyn_cast<VarDecl>(D)) {
if (!VD->hasStorage())
if (!VD->hasStorageOrWrapsStorage())
return None;

// Do not permit potential availability of script-mode global variables;
Expand Down
26 changes: 26 additions & 0 deletions test/Sema/availability_stored.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,55 @@
@available(macOS 50, *)
struct NewStruct {}

@available(macOS 50, *)
@propertyWrapper
struct NewPropertyWrapper<Value> {
var wrappedValue: Value
}

@available(macOS 50, *)
struct GoodReferenceStruct {
var x: NewStruct
@NewPropertyWrapper var y: Int
lazy var z: Int = 42
}

@available(macOS 50, *)
struct GoodNestedReferenceStruct {
struct Inner {
var x: NewStruct
@NewPropertyWrapper var y: Int
lazy var z: Int = 42
}
}

struct BadReferenceStruct1 {
// expected-error@+1 {{stored properties cannot be marked potentially unavailable with '@available'}}
@available(macOS 50, *)
var x: NewStruct

// expected-error@+1 {{stored properties cannot be marked potentially unavailable with '@available'}}
@available(macOS 50, *)
@NewPropertyWrapper var y: Int

// expected-error@+1 {{stored properties cannot be marked potentially unavailable with '@available'}}
@available(macOS 50, *)
lazy var z: Int = 42
}

@available(macOS 40, *)
struct BadReferenceStruct2 {
// expected-error@+1 {{stored properties cannot be marked potentially unavailable with '@available'}}
@available(macOS 50, *)
var x: NewStruct

// expected-error@+1 {{stored properties cannot be marked potentially unavailable with '@available'}}
@available(macOS 50, *)
@NewPropertyWrapper var y: Int

// expected-error@+1 {{stored properties cannot be marked potentially unavailable with '@available'}}
@available(macOS 50, *)
lazy var z: Int = 42
}

// The same behavior should hold for enum elements with payloads.
Expand Down
20 changes: 2 additions & 18 deletions test/Sema/availability_versions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ class ClassWithUnavailableProperties {
@available(OSX, introduced: 10.9)
lazy var availableOn10_9Stored: Int = 9

@available(OSX, introduced: 10.51)
@available(OSX, introduced: 10.51) // expected-error {{stored properties cannot be marked potentially unavailable with '@available'}}
lazy var availableOn10_51Stored : Int = 10

@available(OSX, introduced: 10.9)
Expand Down Expand Up @@ -418,7 +418,6 @@ class ClassWithReferencesInInitializers {
lazy var lazyPropWithInitializer10_51: Int = globalFuncAvailableOn10_51()

lazy var lazyPropWithInitializer10_52: Int = globalFuncAvailableOn10_52() // expected-error {{'globalFuncAvailableOn10_52()' is only available in macOS 10.52 or newer}}
// expected-note@-1 {{add @available attribute to enclosing property}}
}

func accessUnavailableProperties(_ o: ClassWithUnavailableProperties) {
Expand Down Expand Up @@ -716,21 +715,9 @@ class ClassWithDeclarationsOfUnavailableClasses {
// expected-note@-1 5{{add @available attribute to enclosing class}}

@available(OSX, introduced: 10.51)
init() {
unavailablePropertyOfUnavailableType = ClassAvailableOn10_51()
unavailablePropertyOfUnavailableType = ClassAvailableOn10_51()
}
init() {}

var propertyOfUnavailableType: ClassAvailableOn10_51 // expected-error {{'ClassAvailableOn10_51' is only available in macOS 10.51 or newer}}

@available(OSX, introduced: 10.51)
lazy var unavailablePropertyOfUnavailableType: ClassAvailableOn10_51 = ClassAvailableOn10_51()

@available(OSX, introduced: 10.51)
lazy var unavailablePropertyOfOptionalUnavailableType: ClassAvailableOn10_51? = nil

@available(OSX, introduced: 10.51)
lazy var unavailablePropertyOfUnavailableTypeWithInitializer: ClassAvailableOn10_51 = ClassAvailableOn10_51()

@available(OSX, introduced: 10.51)
static var unavailableStaticPropertyOfUnavailableType: ClassAvailableOn10_51 = ClassAvailableOn10_51()
Expand Down Expand Up @@ -1294,15 +1281,12 @@ class ClassForFixit {

lazy var fixitForReferenceInLazyPropertyType: ClassAvailableOn10_51? = nil
// expected-error@-1 {{'ClassAvailableOn10_51' is only available in macOS 10.51 or newer}}
// expected-note@-2 {{add @available attribute to enclosing property}} {{3-3=@available(macOS 10.51, *)\n }}

private lazy var fixitForReferenceInPrivateLazyPropertyType: ClassAvailableOn10_51? = nil
// expected-error@-1 {{'ClassAvailableOn10_51' is only available in macOS 10.51 or newer}}
// expected-note@-2 {{add @available attribute to enclosing property}} {{3-3=@available(macOS 10.51, *)\n }}

lazy private var fixitForReferenceInLazyPrivatePropertyType: ClassAvailableOn10_51? = nil
// expected-error@-1 {{'ClassAvailableOn10_51' is only available in macOS 10.51 or newer}}
// expected-note@-2 {{add @available attribute to enclosing property}} {{3-3=@available(macOS 10.51, *)\n }}

static var fixitForReferenceInStaticPropertyType: ClassAvailableOn10_51? = nil
// expected-error@-1 {{'ClassAvailableOn10_51' is only available in macOS 10.51 or newer}}
Expand Down
1 change: 1 addition & 0 deletions test/decl/class/effectful_properties.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ class AcceptableDynamic {
}

// mainly just some sanity checks
// expected-error@+1 {{class 'Misc' has no initializers}}
class Misc {
// expected-error@+2 {{'lazy' cannot be used on a computed property}}
// expected-error@+1 {{lazy properties must have an initializer}}
Expand Down
18 changes: 9 additions & 9 deletions test/stdlib/CodableTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ class TestCodable : TestCodableSuper {

// MARK: - DateInterval
@available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *)
lazy var dateIntervalValues: [Int : DateInterval] = [
static let dateIntervalValues: [Int : DateInterval] = [
#line : DateInterval(),
#line : DateInterval(start: Date.distantPast, end: Date()),
#line : DateInterval(start: Date(), end: Date.distantFuture),
Expand All @@ -443,14 +443,14 @@ class TestCodable : TestCodableSuper {

@available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *)
func test_DateInterval_JSON() {
for (testLine, interval) in dateIntervalValues {
for (testLine, interval) in Self.dateIntervalValues {
expectRoundTripEqualityThroughJSON(for: interval, lineNumber: testLine)
}
}

@available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *)
func test_DateInterval_Plist() {
for (testLine, interval) in dateIntervalValues {
for (testLine, interval) in Self.dateIntervalValues {
expectRoundTripEqualityThroughPlist(for: interval, lineNumber: testLine)
}
}
Expand Down Expand Up @@ -641,15 +641,15 @@ class TestCodable : TestCodableSuper {

// MARK: - Measurement
@available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *)
lazy var unitValues: [Int : Dimension] = [
static let unitValues: [Int : Dimension] = [
#line : UnitAcceleration.metersPerSecondSquared,
#line : UnitMass.kilograms,
#line : UnitLength.miles
]

@available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *)
func test_Measurement_JSON() {
for (testLine, unit) in unitValues {
for (testLine, unit) in Self.unitValues {
// FIXME: <rdar://problem/49026133>
// Terminating due to uncaught exception NSInvalidArgumentException:
// *** You must override baseUnit in your class NSDimension to define its base unit.
Expand All @@ -660,7 +660,7 @@ class TestCodable : TestCodableSuper {

@available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *)
func test_Measurement_Plist() {
for (testLine, unit) in unitValues {
for (testLine, unit) in Self.unitValues {
// FIXME: <rdar://problem/49026133>
// Terminating due to uncaught exception NSInvalidArgumentException:
// *** You must override baseUnit in your class NSDimension to define its base unit.
Expand Down Expand Up @@ -741,22 +741,22 @@ class TestCodable : TestCodableSuper {

// MARK: - PersonNameComponents
@available(macOS 10.11, iOS 9.0, watchOS 2.0, tvOS 9.0, *)
lazy var personNameComponentsValues: [Int : PersonNameComponents] = [
static let personNameComponentsValues: [Int : PersonNameComponents] = [
#line : makePersonNameComponents(givenName: "John", familyName: "Appleseed"),
#line : makePersonNameComponents(givenName: "John", familyName: "Appleseed", nickname: "Johnny"),
#line : makePersonNameComponents(namePrefix: "Dr.", givenName: "Jane", middleName: "A.", familyName: "Appleseed", nameSuffix: "Esq.", nickname: "Janie")
]

@available(macOS 10.11, iOS 9.0, watchOS 2.0, tvOS 9.0, *)
func test_PersonNameComponents_JSON() {
for (testLine, components) in personNameComponentsValues {
for (testLine, components) in Self.personNameComponentsValues {
expectRoundTripEqualityThroughJSON(for: components, lineNumber: testLine)
}
}

@available(macOS 10.11, iOS 9.0, watchOS 2.0, tvOS 9.0, *)
func test_PersonNameComponents_Plist() {
for (testLine, components) in personNameComponentsValues {
for (testLine, components) in Self.personNameComponentsValues {
expectRoundTripEqualityThroughPlist(for: components, lineNumber: testLine)
}
}
Expand Down