Skip to content

Sema: Diagnose availability of storage accessors in key paths and writebacks #72410

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

Conversation

tshortli
Copy link
Contributor

@tshortli tshortli commented Mar 19, 2024

Builds on #72369.

Fixes the missing diagnostics about the availability of property accessors in the following examples:

struct A {
  struct B {
    var x: Int = 0
  }

  private var _b: B = B()
  var b: B {
    get { _b }
    @available(*, unavailable) set {
      _b = newValue
    }
  }
}

var a = A()
a[keyPath: \.b] = a.b // error: setter for 'b' is unavailable
a.b.x = 1 // error: setter for 'b' is unavailable

Resolves rdar://124977727 and rdar://125019717

@tshortli tshortli force-pushed the key-path-accessor-availability-diagnostics branch from ef1e892 to 3e5bee4 Compare March 19, 2024 04:01
@tshortli tshortli changed the title Diagnose availability of accessors referenced by key paths Sema: Diagnose availability of storage accessors for key paths Mar 19, 2024
@tshortli tshortli marked this pull request as ready for review March 19, 2024 04:02
@tshortli tshortli force-pushed the key-path-accessor-availability-diagnostics branch from 3e5bee4 to 902721a Compare March 19, 2024 17:22
@tshortli tshortli changed the title Sema: Diagnose availability of storage accessors for key paths Sema: Diagnose availability of storage accessors in key paths and writebacks Mar 19, 2024
@tshortli
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

LGTM! I think we might want to consider staging this error as a warning?

@tshortli
Copy link
Contributor Author

LGTM! I think we might want to consider staging this error as a warning?

Thanks! I'd like to see how it goes with an error but I'm prepared to downgrade if necessary.

@xedin
Copy link
Contributor

xedin commented Mar 19, 2024

@swift-ci please test source compatibility

@xedin
Copy link
Contributor

xedin commented Mar 19, 2024

Let's see if any of our source compatibility suite projects have this problem.

@tshortli
Copy link
Contributor Author

Let's see if any of our source compatibility suite projects have this problem.

I checked the results and there are no regressions (there are some known issues from non-copyable generics adoption).

@xedin
Copy link
Contributor

xedin commented Mar 19, 2024

Great, thank you!

Fixes the missing diagnostic about the availability of the setter of `x` in the
following example:

```
struct S {
  var x: Int {
    get { 0 }
    @available(*, unavailable) set {}
  }
}

var s = S()
s[keyPath: \.x] = 1
```

Resolves rdar://124977727
In the following example, the writeback via the property `b` should be
diagnosed since `b`'s setter is unavailable:

```
struct A {
  struct B {
    var x: Int = 0
  }

  private var _b: B = B()
  var b: B {
    get { _b }
    @available(*, unavailable) set {
      _b = newValue
    }
  }
}

var a = A()
a.b.x = 1
```

Resolves rdar://125019717
@tshortli tshortli force-pushed the key-path-accessor-availability-diagnostics branch from 902721a to c7f0c58 Compare March 20, 2024 00:11
@tshortli
Copy link
Contributor Author

@swift-ci please smoke test

@tshortli tshortli enabled auto-merge March 20, 2024 00:12
@tshortli
Copy link
Contributor Author

@swift-ci please smoke test Windows

@tshortli tshortli merged commit f3441c0 into swiftlang:main Mar 20, 2024
@tshortli tshortli deleted the key-path-accessor-availability-diagnostics branch March 20, 2024 15:06
tshortli added a commit to tshortli/swift that referenced this pull request Jun 27, 2024
The changes in swiftlang#72410 caused a regression
when checking availability in the following example:

```
// warning: Setter for 'hasDeprecatedSetter' is deprecated: ...
x[y.hasDeprecatedSetter] = ...
```

The result of `y.hasDeprecatedSetter` is being passed as an argument to the
subscript and its setter will not be called. To fix this,
`ExprAvailabilityWalker` now consistently creates a new default
`MemberAccessContext` when descending into any `Argument`, since the access
context for the expressions surrounding the call should not affect the
arguments to the call.

Additionally, `MemberAccessContext` has been refactored to better model context
state transitions. Instead of only modeling which accessors will be called, the
enumeration's members now reflect the possible states that
`ExprAvailabilityWalker` can be in during its traversal. This should hopefully
make it easier to follow the logic for traversal of `LoadExpr`s and arguments.

Resolves rdar://130487998.
tshortli added a commit to tshortli/swift that referenced this pull request Jun 27, 2024
The changes in swiftlang#72410 caused a regression
when checking availability in the following example:

```
// warning: Setter for 'hasDeprecatedSetter' is deprecated: ...
x[y.hasDeprecatedSetter] = ...
```

The result of `y.hasDeprecatedSetter` is being passed as an argument to the
subscript and its setter will not be called. To fix this,
`ExprAvailabilityWalker` now consistently creates a new default
`MemberAccessContext` when descending into any `Argument`, since the access
context for the expressions surrounding the call should not affect the
arguments to the call.

Additionally, `MemberAccessContext` has been refactored to better model context
state transitions. Instead of only modeling which accessors will be called, the
enumeration's members now reflect the possible states that
`ExprAvailabilityWalker` can be in during its traversal. This should hopefully
make it easier to follow the logic for traversal of `LoadExpr`s and arguments.

Resolves rdar://130487998.
tshortli added a commit to tshortli/swift that referenced this pull request Jun 27, 2024
The changes in swiftlang#72410 caused a regression
when checking availability in the following example:

```
// warning: Setter for 'hasDeprecatedSetter' is deprecated: ...
x[y.hasDeprecatedSetter] = ...
```

The result of `y.hasDeprecatedSetter` is being passed as an argument to the
subscript and its setter will not be called. To fix this,
`ExprAvailabilityWalker` now consistently creates a new default
`MemberAccessContext` when descending into any `Argument`, since the access
context for the expressions surrounding the call should not affect the
arguments to the call.

Additionally, `MemberAccessContext` has been refactored to better model context
state transitions. Instead of only modeling which accessors will be called, the
enumeration's members now reflect the possible states that
`ExprAvailabilityWalker` can be in during its traversal. This should hopefully
make it easier to follow the logic for traversal of `LoadExpr`s and arguments.

Resolves rdar://130487998.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants