[Review] SE-0153: Compensate for the inconsistency of @NSCopying's behaviour


(Douglas Gregor) #1

Hello Swift community,

The review of SE-0153 "Compensate for the inconsistency of @NSCopying's behaviour" begins now and runs through February 22, 2017. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0153-compensate-for-the-inconsistency-of-nscopyings-behaviour.md
Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0153-compensate-for-the-inconsistency-of-nscopyings-behaviour.md
Reply text

Other replies
<https://github.com/apple/swift-evolution/pulls#what-goes-into-a-review-1>What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,

-Doug Gregor

Review Manager


(Xiaodi Wu) #2

   - What is your evaluation of the proposal?

This document seems to propose two contradictory alternative solutions; I

assume the goal is that the core team will choose one of two. I'm not sure
that either is an improvement over the status quo, for reasons I outline
below.

   - Is the problem being addressed significant enough to warrant a
   change to Swift?

I agree that the current situation is problematic because of

inconsistency, but I think both proposed solutions are more problematic
because of more inconsistency.

   - Does this proposal fit well with the feel and direction of Swift?

On the one hand, I can agree that `@NSCopying` not being respected in

`init()` can be confusing. However, as was pointed out during the initial
pitch, this is consistent with other behaviors. For example, `didSet` is
not called from `init()` either, and there are good reasons for this. If
the proposal for custom behaviors were to come back into consideration, I
would assume that none of those behaviors could be triggered from `init()`
either.

A person who is new to Swift would continue to be confused if `@NSCopying`
had magic but `didSet` and other behaviors did not. A person who has
studied Swift and internalized the reasoning behind this initially tricky
situation might rightly expect that _all_ behaviors, including
`@NSCopying`, are ignored during `init`.

The proposal seems to prioritize new users migrating from Obj-C, who are
unfamiliar with Swift idioms, over Swift users who are right to expect some
internal consistency in the language. While supporting both groups is
important, I'm not sure it's appropriate to increase inconsistency within
Swift itself to help with migration from Obj-C.

···

   - If you have used other languages or libraries with a similar
   feature, how do you feel that this proposal compares to those?

N/A.

   - How much effort did you put into your review? A glance, a quick
   reading, or an in-depth study?

A quick reading.


(David Waite) #3

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0153-compensate-for-the-inconsistency-of-nscopyings-behaviour.md

What is your evaluation of the proposal?

Its an ugly problem. My preference would be to warn the developer to call copy manually for now, vs copy automatically.

The reason I’m concerned about the automatic approach is that I do not know if property behaviors are something which may still appear in a future version of Swift. At that point, I would expect NSCopying to become a behavior, and not a special case for initializer code generation.

If property behaviors are not planned for any future version of swift, then I suppose I’m impartial to the solution.

Is the problem being addressed significant enough to warrant a change to Swift?

I think so; it won’t be clear to developers that initializers special-case property access, and silently going against their declared desired behavior is a bad idea.

Does this proposal fit well with the feel and direction of Swift?

As stated above, I believe it depends on whether or not there is a future property behaviors spec that this would conflict with.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

In Ruby, the behavior is simpler - @foo will access the property value directly, while self.foo or some_reference.foo will use the ‘foo’ function. Dots are always used to invoke functions.

Objective C has a syntax for property access as well, in that you can refer to the synthesized property values directly via an underscore prefix.

A syntax for direct property data access in swift would be a third way to solve the problem. In that case, it would be clearer to the developer what they needed to do.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

A quick reading

-DW


(Brent Royal-Gordon) #4

https://github.com/apple/swift-evolution/blob/master/proposals/0153-compensate-for-the-inconsistency-of-nscopyings-behaviour.md

  • What is your evaluation of the proposal?

I think a warning is very appropriate.

However, occasionally a constructor might be non-copying on purpose, so we'll need a way to disable the warning. A few possibilities:

* Use .self on the value
* Parenthesize the value
* Parenthesize the variable

I don't favor using compiler magic. Too spooky.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yeah, that's a pretty easy mistake to make.

  • Does this proposal fit well with the feel and direction of Swift?

Yes. Swift usually tries to help you avoid mistakes.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading.

···

On Feb 17, 2017, at 3:18 PM, Douglas Gregor <dgregor@apple.com> wrote:

--
Brent Royal-Gordon
Architechies


(Rod Brown) #5

What is your evaluation of the proposal?

It’s clearly a concern and one that should be addressed.

A lot of people have bought up the didSet {} property observers are not called when things are set here, either. This is true, but unlike property observers, these are attributes, and I don’t know of anywhere else where an @attribute works in some cases and not in another. It is understandable that people think @NSCopying is implemented with compiler magic that should work everywhere, rather than built as part of the setter as it currently is, and thus causes confusion.

Like others have commented, I am concerned about how our solution works with future plans about making NSCopying a property behaviour. Doug Gregor has commented that he believes it should be compiler magic, and I respect that opinion. I would like the Core Team to ensure that they believe it is entirely compatible and in the spirit of Property Behaviours before the compiler magic is added, though, if Property Behaviours are still seriously on the cards for the future.

If compiler magic is decided against, I think this is a clear spot where there should be a warning, and a proposed fixit.

Is the problem being addressed significant enough to warrant a change to Swift?

Yes. This is a confusing and easily missed part of the language. While it is currently understood that didSet and willSet are not called, it is not clear to developers that @NSCopying falls into that same group, and have not noticed it anywhere in documentation.

Does this proposal fit well with the feel and direction of Swift?

I think adding compiler magic is somewhat in opposition to the general trend to reduce said magic, but a solution here seems necessary as the current lack of clarity is against the direction of Swift.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

A quick reading of the proposal, and involved in the initial discussions surrounding this issue.


(David Hart) #6

What is your evaluation of the proposal?

For the same reasons as Xiaodi, this proposal could be potentially misleading if it introduces custom compiler magic, warning or errors that was not replicated for future property behaviours.

But at the same time, it's very easy for even a seasoned developer to always remember to do the right thing in initializers. Swift's safety goals should technically avoid such simple mistakes from being made.

So I'm torn, with a slight preference for accepting the proposal with the warning solution.

Is the problem being addressed significant enough to warrant a change to Swift?

It's a safety concern that can avoid bugs in our code so I'd say it's significant enough.

Does this proposal fit well with the feel and direction of Swift?

Yes.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

No.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading.

···

On 18 Feb 2017, at 02:05, Xiaodi Wu via swift-evolution <swift-evolution@swift.org> wrote:

What is your evaluation of the proposal?

This document seems to propose two contradictory alternative solutions; I assume the goal is that the core team will choose one of two. I'm not sure that either is an improvement over the status quo, for reasons I outline below.

Is the problem being addressed significant enough to warrant a change to Swift?

I agree that the current situation is problematic because of inconsistency, but I think both proposed solutions are more problematic because of more inconsistency.

Does this proposal fit well with the feel and direction of Swift?

On the one hand, I can agree that `@NSCopying` not being respected in `init()` can be confusing. However, as was pointed out during the initial pitch, this is consistent with other behaviors. For example, `didSet` is not called from `init()` either, and there are good reasons for this. If the proposal for custom behaviors were to come back into consideration, I would assume that none of those behaviors could be triggered from `init()` either.

A person who is new to Swift would continue to be confused if `@NSCopying` had magic but `didSet` and other behaviors did not. A person who has studied Swift and internalized the reasoning behind this initially tricky situation might rightly expect that _all_ behaviors, including `@NSCopying`, are ignored during `init`.

The proposal seems to prioritize new users migrating from Obj-C, who are unfamiliar with Swift idioms, over Swift users who are right to expect some internal consistency in the language. While supporting both groups is important, I'm not sure it's appropriate to increase inconsistency within Swift itself to help with migration from Obj-C.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

A quick reading.

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Torin Kwok) #7

Hello Swift community:

Thank you for your reviews for my proposal. As you may have seen, this proposal contains two solutions originally:

Compiler magic
Compile-time checking
However even though Solution: Compiler magic is viable, according to some of feedback from several reviewers, a developer who is new to Swift would continue to be misled by the introduction of the automatic copy behavior:

from Xiaodi Wu xiaodi.wu@gmail.com <mailto:xiaodi.wu@gmail.com>:

A person who is new to Swift would continue to be confused if @NSCopying had magic but didSet and other behaviors did not. A person who has studied Swift and internalized the reasoning behind this initially tricky situation might rightly expect that all behaviors, including @NSCopying, are ignored during init.
from David Hart david@hartbit.com <mailto:david@hartbit.com>:

For the same reasons as Xiaodi, this proposal could be potentially misleading if it introduces custom compiler magic, warning or errors that was not replicated for future property behaviours.
For this consideration, I decided to lower the priority of Solution: Compiler magic to Alternatives Considered section and leave Solution: Compile-time checking as the only one major solution, which suggests that compiler warns the developers to call copy manually, rather than implicit magic:

Have compiler emit a compile-time error or warning if developers are performing an assignment operation from within an initializer between a property declared as @NSCopying and an instance of a protocol conforming class.
I’ve already submitted a pull-request for this modification.

Best,

Torin


(Torin Kwok) #8

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0153-compensate-for-the-inconsistency-of-nscopyings-behaviour.md
Reply text:

However, occasionally a constructor might be non-copying on purpose, so we'll need a way to disable the warning.
How about the determination method mentioned in the proposal:

... if developers are performing an assignment operation from within an initializer between a property declared as @NSCopyingand an instance of a protocol conforming class.
That is, there are two key factors determining whether a fixable error or warning should be emitted:

If the destination property has been declared as @NSCopying in class definition
If the source value conforms to <NSCopying> protocol
Once both are true, then what compiler can be sure about is that developer definitely missed the required manual copy invocation and it would emit either an error or a warning correspondingly. Otherwise, compiler would not complain of anything and would just leave the decision up to developers.

···

On 18 Feb 2017, at 18:16, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

On Feb 17, 2017, at 3:18 PM, Douglas Gregor <dgregor@apple.com> wrote:

https://github.com/apple/swift-evolution/blob/master/proposals/0153-compensate-for-the-inconsistency-of-nscopyings-behaviour.md

  • What is your evaluation of the proposal?

I think a warning is very appropriate.

However, occasionally a constructor might be non-copying on purpose, so we'll need a way to disable the warning. A few possibilities:

* Use .self on the value
* Parenthesize the value
* Parenthesize the variable

I don't favor using compiler magic. Too spooky.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yeah, that's a pretty easy mistake to make.

  • Does this proposal fit well with the feel and direction of Swift?

Yes. Swift usually tries to help you avoid mistakes.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Torin Kwok) #9

I have already submitted a new pull-request <https://github.com/apple/swift-evolution/pull/617> for this modification to the proposal SE-0153. Please you guys feel free to submit your review on GitHub's File Changed page <https://github.com/apple/swift-evolution/pull/617/files> or just reply this mail directly. Thank you.

···

On 18 Feb 2017, at 19:57, Torin Kwok <torin@kwok.im> wrote:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0153-compensate-for-the-inconsistency-of-nscopyings-behaviour.md
Reply text:

However, occasionally a constructor might be non-copying on purpose, so we'll need a way to disable the warning.
How about the determination method mentioned in the proposal:

... if developers are performing an assignment operation from within an initializer between a property declared as @NSCopyingand an instance of a protocol conforming class.
That is, there are two key factors determining whether a fixable error or warning should be emitted:

If the destination property has been declared as @NSCopying in class definition
If the source value conforms to <NSCopying> protocol
Once both are true, then what compiler can be sure about is that developer definitely missed the required manual copy invocation and it would emit either an error or a warning correspondingly. Otherwise, compiler would not complain of anything and would just leave the decision up to developers.

On 18 Feb 2017, at 18:16, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

On Feb 17, 2017, at 3:18 PM, Douglas Gregor <dgregor@apple.com <mailto:dgregor@apple.com>> wrote:

https://github.com/apple/swift-evolution/blob/master/proposals/0153-compensate-for-the-inconsistency-of-nscopyings-behaviour.md

  • What is your evaluation of the proposal?

I think a warning is very appropriate.

However, occasionally a constructor might be non-copying on purpose, so we'll need a way to disable the warning. A few possibilities:

* Use .self on the value
* Parenthesize the value
* Parenthesize the variable

I don't favor using compiler magic. Too spooky.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yeah, that's a pretty easy mistake to make.

  • Does this proposal fit well with the feel and direction of Swift?

Yes. Swift usually tries to help you avoid mistakes.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution