Pitch: Omit deprecation warnings for same-file references


(Tony Allevato) #1

Hey Swift Evolvers,

I'd like to propose a change that would suppress deprecation warnings when
the reference to the deprecated declaration is in the same file as that
declaration.

This personally affects one of my projects (Swift protocol buffers) because
.proto files allow declarations to be declared as deprecated, and we'd like
to surface that by deprecating the equivalent declaration in the Swift code
that we generate. We can't do this without generating noisy build logs,
because we still need to reference the deprecated declarations to
encode/decode the messages correctly; and the workarounds have significant
performance penalties as described in the draft.

I'd love some feedback from folks to know whether this—or something like
it—is something that seems desirable/undesirable for the language. I've
tinkered with an implementation in a branch
<https://github.com/apple/swift/compare/master...allevato:omit-deprecation-same-file>
and it's fairly straightforward.

Gist link: https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb

Omit deprecation warnings for same-file references

   - Proposal: SE-0000
   <https://github.com/apple/swift-evolution/blob/master/proposals/NNNN-name.md>
   - Author(s): Tony Allevato <https://github.com/allevato>
   - Status: Awaiting review
   <https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#rationale>
   - Review manager: TBD

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#introduction>
Introduction

Public API developers can use the @available attribute in Swift to mark
APIs as deprecated, which will emit warnings when a user references that
API to notify them that that particular API is no longer preferred or may
disappear in the future.

These warnings are emitted for *any* reference to a deprecated entity,
including those in the same file. In some cases, however, it may be
necessary and correct to continue referring to the deprecated entity
privately while discouraging its external use. In these scenarios, the
warnings are superfluous and generate noise in the build logs. We propose
eliminating these warnings for references made in the same file.

Swift-evolution thread: TBD
<http://thread.gmane.org/gmane.comp.lang.swift.evolution/>
<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#motivation>
Motivation

As APIs evolve, declarations may become deprecated but still need to be
referenced privately in order to function correctly. For example, consider
an object that is serialized/parsed between a client and server. The first
iteration of such a type might look like this (the examples use a strawman
API to avoid tying the discussion too closely with the new coding APIs,
which is not the focus):

public class Person {
  public var name: String
  public var phoneNumber: String?

  public init(name: String, phoneNumber: String? = nil) {
    self.name = name
    self.phoneNumber = phoneNumber
  }

  public convenience init?(from reader: Reader) {
    guard let name = reader.readString(withKey: "name") else {
      return nil
    }
    let phoneNumber = reader.readString(withKey: "phoneNumber")
    self.init(name: name, phoneNumber: phoneNumber)
  }

  public func write(to writer: Writer) {
    writer.write(name, key: "name")
    if let phoneNumber = phoneNumber {
      writer.write(phoneNumber, key: "phoneNumber")
    }
  }
}

Later, we decide that we need to support storing multiple phone numbers for
a Person. To avoid breaking source compatibility, we deprecate the old
property and add a new one. We still wish the encoding/decoding process to
preserve the integrity of the old data, however—for example, a middleman
process used for logging should not modify the data stream, so the
encoding/decoding process should not be also migrating the data. Thus, we
update the class to the following:

public class Person {
  public var name: String
  @available(*, deprecated, message: "use 'phoneNumbers' instead")
  public var phoneNumber: String?
  public var phoneNumbers: [String]

  @available(*, deprecated, message: "use 'init(name:phoneNumbers:)' instead")
  public convenience init(name: String, phoneNumber: String?) {
    self.phoneNumber = phoneNumber
    self.init(name: name, phoneNumbers: [])
  }

  public init(name: String, phoneNumbers: [String] = []) {
    self.name = name
    self.phoneNumber = nil
    self.phoneNumbers = phoneNumbers
  }

  public convenience init?(from reader: Reader) {
    guard let name = reader.readString(withKey: "name") else {
      return nil
    }
    let phoneNumbers = reader.readStringArray(withKey: "phoneNumbers") ?? []
    self.phoneNumber = reader.readString(withKey: "phoneNumber")
    self.init(name: name, phoneNumbers: phoneNumbers)
  }

  public func write(to writer: Writer) {
    writer.write(name, key: "name")
    if let phoneNumber = phoneNumber {
      writer.write(phoneNumber, key: "phoneNumber")
    }
    if !phoneNumbers.isEmpty {
      writer.write(phoneNumbers, key: "phoneNumbers")
    }
  }
}

This type is backwards-compatible with the previous version and will warn
external users that they should migrate to the new version of the API; it
will also have its values preserved exactly if a middleman process parses
and then reserializes it.

The problem, however, is that the references to phoneNumber in Person's *own
methods* also emit deprecation warnings. This is undesirable because the
warnings cannot be easily avoided (see workarounds below). The crux of the
issue is that one of the two statements is likely to be true:

   1.

   At the time the user deprecates something, they also have the
   opportunity to migrate all references to it within the same file, thus
   eliminating the warnings there and making the file build cleanly.
   2.

   At the time the user deprecates something, they must continue to
   reference it internally to achieve correct and performant behavior, thus
   making it currently impossible to achieve a clean build for the file
   containing that declaration.

This proposal aims to solve the second issue.
<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#workarounds-and-their-problems>Workarounds
and their problems

One workaround would be to shadow deprecated declarations by making the
declaration itself private (and prepending something like an underscore to
the name) and then using a public passthrough property or method. The
private declaration would not be deprecated while the public one would be.

There are two main problems with this approach. First, it increases the
development and maintanence cost of the deprecation because the new
declaration has to be added and all internal references to the declaration
must be updated.

Secondly, and more importantly, this shadowing can have significant
performance penalties. Consider this example:

class DirectArrayHolder {
  @available(*, deprecated, message: "Use something else instead")
  var array: [String] = []
}
class IndirectArrayHolder {
  var _array: [String] = []

  @available(*, deprecated, message: "Use something else instead")
  var array: [String] {
    get { return _array }
    set { _array = newValue }
  }
}
func useDirectHolder() -> DirectArrayHolder {
  let holder = DirectArrayHolder()
  for _ in 0...10_000 {
    holder.array.append("foo")
  }
  return holder
}
func useIndirectHolder() -> IndirectArrayHolder {
  let holder = IndirectArrayHolder()
  for _ in 0...10_000 {
    holder.array.append("foo")
  }
  return holder
}

Behaviorally, these are the same. However, the generated code is different
because the memory management semantics change due to the passthrough
property. With full optimizations on, useDirectHolder executes in 1.360 sec
while useIndirectHolder executes in 235.8 sec—over two orders of magnitude
slower!

It could be argued that this performance issue is something that could be
solved separately, but this should be considered just one motivating
example and not the sole motivation for this proposed change.
<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#proposed-solution>Proposed
solution

Do not print deprecation warnings for references to deprecated declarations
that are in the same file.

There would be no change for declarations marked unavailable. A declaration
marked unavailable is making a stronger statement about its existence such
that referencing it is an error; unlike marking a declaration deprecated,
marking it unavailable is a breaking change for downstream clients, so the
behavior of that attribute value should be unchanged whether references are
in the same file or elsewhere.
<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#implementation-details>Implementation
details

A small addition would be made to TypeChecker::diagnoseIfDeprecated so that
it returns early if the declaration is in the same source file as the
reference.
<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#impact-on-existing-code>Impact
on existing code

This is not a source-breaking change. The only side effect for existing
code is that deprecation warnings that were emitted for same-file
references in the past will no longer be emitted.
<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#alternatives-considered>Alternatives
considered

Omitting deprecation warnings for references in the same *module*, not only
the same *file*, was also considered. An argument could be made that
deprecation warnings are intended as messaging for external users of an
API, so drawing the line at the module level would be more appropriate than
at the file level. However, modules can be large and span a broad set of
APIs contributed by many developers on multi-person teams, and they may
want to use the @available attribute to phase out particular APIs for
internal use as well. Omitting deprecation warnings for anything in the
same module takes that useful tool away for those users.

Another idea that was brought up in a feature request
<https://bugs.swift.org/browse/SR-3357> filed for this was to allow a
"deprecated block" that would suppress all deprecation warnings inside it.
This would also solve the problem motivating this proposal and is similar
to the approach taken by other languages (such as Java's
@SuppressWarnings("deprecation") annotation), but this approach has
drawbacks as well. It would not distinguish between deprecation warnings in
the same file or module vs. those in other files or modules (unless
modifiers were added to it, which runs the risk of adding too much
complexity for too little gain). Furthermore, it has the potential to be
harmful if abused—developers could surround large code blocks with the
deprecation suppressor and then lose any deprecation warnings that would
have been emitted for APIs used inside that block.


Treat warnings as errors except deprecations
(BJ Homer) #2

I’ve run into this problem as well, when dealing with legacy file formats (similar to the example in the proposal), and I agree it would be nice to be able to address it. We’ve got a few “permanent” warnings in our code base right now due to this. I’m not sure whether the deprecation should be disabled for the entire file, though; perhaps it should be disabled just within the type itself? I don’t know how that would contribute to the complexity of the implementation, but it seems like we would still want to warn about deprecated accesses between two types declared in the same file.

-BJ Homer

···

On May 5, 2017, at 12:12 PM, Tony Allevato via swift-evolution <swift-evolution@swift.org> wrote:

Hey Swift Evolvers,

I'd like to propose a change that would suppress deprecation warnings when the reference to the deprecated declaration is in the same file as that declaration.

This personally affects one of my projects (Swift protocol buffers) because .proto files allow declarations to be declared as deprecated, and we'd like to surface that by deprecating the equivalent declaration in the Swift code that we generate. We can't do this without generating noisy build logs, because we still need to reference the deprecated declarations to encode/decode the messages correctly; and the workarounds have significant performance penalties as described in the draft.

I'd love some feedback from folks to know whether this—or something like it—is something that seems desirable/undesirable for the language. I've tinkered with an implementation in a branch <https://github.com/apple/swift/compare/master...allevato:omit-deprecation-same-file> and it's fairly straightforward.

Gist link: https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb

Omit deprecation warnings for same-file references

Proposal: SE-0000 <https://github.com/apple/swift-evolution/blob/master/proposals/NNNN-name.md>
Author(s): Tony Allevato <https://github.com/allevato>
Status: Awaiting review <https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#rationale>
Review manager: TBD
<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#introduction>Introduction

Public API developers can use the @available attribute in Swift to mark APIs as deprecated, which will emit warnings when a user references that API to notify them that that particular API is no longer preferred or may disappear in the future.

These warnings are emitted for any reference to a deprecated entity, including those in the same file. In some cases, however, it may be necessary and correct to continue referring to the deprecated entity privately while discouraging its external use. In these scenarios, the warnings are superfluous and generate noise in the build logs. We propose eliminating these warnings for references made in the same file.

Swift-evolution thread: TBD <http://thread.gmane.org/gmane.comp.lang.swift.evolution/>
<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#motivation>Motivation

As APIs evolve, declarations may become deprecated but still need to be referenced privately in order to function correctly. For example, consider an object that is serialized/parsed between a client and server. The first iteration of such a type might look like this (the examples use a strawman API to avoid tying the discussion too closely with the new coding APIs, which is not the focus):

public class Person {
  public var name: String
  public var phoneNumber: String?

  public init(name: String, phoneNumber: String? = nil) {
    self.name = name
    self.phoneNumber = phoneNumber
  }

  public convenience init?(from reader: Reader) {
    guard let name = reader.readString(withKey: "name") else {
      return nil
    }
    let phoneNumber = reader.readString(withKey: "phoneNumber")
    self.init(name: name, phoneNumber: phoneNumber)
  }

  public func write(to writer: Writer) {
    writer.write(name, key: "name")
    if let phoneNumber = phoneNumber {
      writer.write(phoneNumber, key: "phoneNumber")
    }
  }
}
Later, we decide that we need to support storing multiple phone numbers for a Person. To avoid breaking source compatibility, we deprecate the old property and add a new one. We still wish the encoding/decoding process to preserve the integrity of the old data, however—for example, a middleman process used for logging should not modify the data stream, so the encoding/decoding process should not be also migrating the data. Thus, we update the class to the following:

public class Person {
  public var name: String
  @available(*, deprecated, message: "use 'phoneNumbers' instead")
  public var phoneNumber: String?
  public var phoneNumbers: [String]

  @available(*, deprecated, message: "use 'init(name:phoneNumbers:)' instead")
  public convenience init(name: String, phoneNumber: String?) {
    self.phoneNumber = phoneNumber
    self.init(name: name, phoneNumbers: [])
  }

  public init(name: String, phoneNumbers: [String] = []) {
    self.name = name
    self.phoneNumber = nil
    self.phoneNumbers = phoneNumbers
  }

  public convenience init?(from reader: Reader) {
    guard let name = reader.readString(withKey: "name") else {
      return nil
    }
    let phoneNumbers = reader.readStringArray(withKey: "phoneNumbers") ?? []
    self.phoneNumber = reader.readString(withKey: "phoneNumber")
    self.init(name: name, phoneNumbers: phoneNumbers)
  }

  public func write(to writer: Writer) {
    writer.write(name, key: "name")
    if let phoneNumber = phoneNumber {
      writer.write(phoneNumber, key: "phoneNumber")
    }
    if !phoneNumbers.isEmpty {
      writer.write(phoneNumbers, key: "phoneNumbers")
    }
  }
}
This type is backwards-compatible with the previous version and will warn external users that they should migrate to the new version of the API; it will also have its values preserved exactly if a middleman process parses and then reserializes it.

The problem, however, is that the references to phoneNumber in Person's own methods also emit deprecation warnings. This is undesirable because the warnings cannot be easily avoided (see workarounds below). The crux of the issue is that one of the two statements is likely to be true:

At the time the user deprecates something, they also have the opportunity to migrate all references to it within the same file, thus eliminating the warnings there and making the file build cleanly.

At the time the user deprecates something, they must continue to reference it internally to achieve correct and performant behavior, thus making it currently impossible to achieve a clean build for the file containing that declaration.

This proposal aims to solve the second issue.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#workarounds-and-their-problems>Workarounds and their problems

One workaround would be to shadow deprecated declarations by making the declaration itself private (and prepending something like an underscore to the name) and then using a public passthrough property or method. The private declaration would not be deprecated while the public one would be.

There are two main problems with this approach. First, it increases the development and maintanence cost of the deprecation because the new declaration has to be added and all internal references to the declaration must be updated.

Secondly, and more importantly, this shadowing can have significant performance penalties. Consider this example:

class DirectArrayHolder {
  @available(*, deprecated, message: "Use something else instead")
  var array: [String] = []
}

class IndirectArrayHolder {
  var _array: [String] = []

  @available(*, deprecated, message: "Use something else instead")
  var array: [String] {
    get { return _array }
    set { _array = newValue }
  }
}

func useDirectHolder() -> DirectArrayHolder {
  let holder = DirectArrayHolder()
  for _ in 0...10_000 {
    holder.array.append("foo")
  }
  return holder
}

func useIndirectHolder() -> IndirectArrayHolder {
  let holder = IndirectArrayHolder()
  for _ in 0...10_000 {
    holder.array.append("foo")
  }
  return holder
}
Behaviorally, these are the same. However, the generated code is different because the memory management semantics change due to the passthrough property. With full optimizations on, useDirectHolder executes in 1.360 sec while useIndirectHolder executes in 235.8 sec—over two orders of magnitude slower!

It could be argued that this performance issue is something that could be solved separately, but this should be considered just one motivating example and not the sole motivation for this proposed change.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#proposed-solution>Proposed solution

Do not print deprecation warnings for references to deprecated declarations that are in the same file.

There would be no change for declarations marked unavailable. A declaration marked unavailable is making a stronger statement about its existence such that referencing it is an error; unlike marking a declaration deprecated, marking it unavailable is a breaking change for downstream clients, so the behavior of that attribute value should be unchanged whether references are in the same file or elsewhere.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#implementation-details>Implementation details

A small addition would be made to TypeChecker::diagnoseIfDeprecated so that it returns early if the declaration is in the same source file as the reference.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#impact-on-existing-code>Impact on existing code

This is not a source-breaking change. The only side effect for existing code is that deprecation warnings that were emitted for same-file references in the past will no longer be emitted.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#alternatives-considered>Alternatives considered

Omitting deprecation warnings for references in the same module, not only the same file, was also considered. An argument could be made that deprecation warnings are intended as messaging for external users of an API, so drawing the line at the module level would be more appropriate than at the file level. However, modules can be large and span a broad set of APIs contributed by many developers on multi-person teams, and they may want to use the @available attribute to phase out particular APIs for internal use as well. Omitting deprecation warnings for anything in the same module takes that useful tool away for those users.

Another idea that was brought up in a feature request <https://bugs.swift.org/browse/SR-3357> filed for this was to allow a "deprecated block" that would suppress all deprecation warnings inside it. This would also solve the problem motivating this proposal and is similar to the approach taken by other languages (such as Java's @SuppressWarnings("deprecation") annotation), but this approach has drawbacks as well. It would not distinguish between deprecation warnings in the same file or module vs. those in other files or modules (unless modifiers were added to it, which runs the risk of adding too much complexity for too little gain). Furthermore, it has the potential to be harmful if abused—developers could surround large code blocks with the deprecation suppressor and then lose any deprecation warnings that would have been emitted for APIs used inside that block.

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


(Tony Allevato) #3

I’ve run into this problem as well, when dealing with legacy file formats
(similar to the example in the proposal), and I agree it would be nice to
be able to address it. We’ve got a few “permanent” warnings in our code
base right now due to this. I’m not sure whether the deprecation should be
disabled for the entire file, though; perhaps it should be disabled just
within the type itself? I don’t know how that would contribute to the
complexity of the implementation, but it seems like we would still want to
warn about deprecated accesses between two types declared in the same file.

I initially picked "same file" because (1) it appears to be dead simple to
implement and (2) the idea that if someone is marking a definition as
private in a file, they can easily go through the same file and resolve any
uses of that declaration if they no longer want them. But it's possible
that there are helper types that just happen to be in the same file where
you'd like to be warned about using those "privately allowed APIs", so
you're right that same-file suppression would make those harder to detect.

Given that, I'd be open to either of these possibilities and I'd love to
hear what others think:

* Give deprecation-suppression "fileprivate" semantics
* Give deprecation-suppression "private" semantics (suppressed in the same
type and in extensions of that type within the same file)

···

On Fri, May 5, 2017 at 11:37 AM BJ Homer <bjhomer@gmail.com> wrote:

-BJ Homer

On May 5, 2017, at 12:12 PM, Tony Allevato via swift-evolution < > swift-evolution@swift.org> wrote:

Hey Swift Evolvers,

I'd like to propose a change that would suppress deprecation warnings when
the reference to the deprecated declaration is in the same file as that
declaration.

This personally affects one of my projects (Swift protocol buffers)
because .proto files allow declarations to be declared as deprecated, and
we'd like to surface that by deprecating the equivalent declaration in the
Swift code that we generate. We can't do this without generating noisy
build logs, because we still need to reference the deprecated declarations
to encode/decode the messages correctly; and the workarounds have
significant performance penalties as described in the draft.

I'd love some feedback from folks to know whether this—or something like
it—is something that seems desirable/undesirable for the language. I've
tinkered with an implementation in a branch
<https://github.com/apple/swift/compare/master...allevato:omit-deprecation-same-file>
and it's fairly straightforward.

Gist link:
https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb

Omit deprecation warnings for same-file references

   - Proposal: SE-0000
   <https://github.com/apple/swift-evolution/blob/master/proposals/NNNN-name.md>
   - Author(s): Tony Allevato <https://github.com/allevato>
   - Status: Awaiting review
   <https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#rationale>
   - Review manager: TBD

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#introduction>
Introduction

Public API developers can use the @available attribute in Swift to mark
APIs as deprecated, which will emit warnings when a user references that
API to notify them that that particular API is no longer preferred or may
disappear in the future.

These warnings are emitted for *any* reference to a deprecated entity,
including those in the same file. In some cases, however, it may be
necessary and correct to continue referring to the deprecated entity
privately while discouraging its external use. In these scenarios, the
warnings are superfluous and generate noise in the build logs. We propose
eliminating these warnings for references made in the same file.

Swift-evolution thread: TBD
<http://thread.gmane.org/gmane.comp.lang.swift.evolution/>

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#motivation>
Motivation

As APIs evolve, declarations may become deprecated but still need to be
referenced privately in order to function correctly. For example, consider
an object that is serialized/parsed between a client and server. The first
iteration of such a type might look like this (the examples use a strawman
API to avoid tying the discussion too closely with the new coding APIs,
which is not the focus):

public class Person {
  public var name: String
  public var phoneNumber: String?

  public init(name: String, phoneNumber: String? = nil) {
    self.name = name
    self.phoneNumber = phoneNumber
  }

  public convenience init?(from reader: Reader) {
    guard let name = reader.readString(withKey: "name") else {
      return nil
    }
    let phoneNumber = reader.readString(withKey: "phoneNumber")
    self.init(name: name, phoneNumber: phoneNumber)
  }

  public func write(to writer: Writer) {
    writer.write(name, key: "name")
    if let phoneNumber = phoneNumber {
      writer.write(phoneNumber, key: "phoneNumber")
    }
  }
}

Later, we decide that we need to support storing multiple phone numbers
for a Person. To avoid breaking source compatibility, we deprecate the
old property and add a new one. We still wish the encoding/decoding process
to preserve the integrity of the old data, however—for example, a middleman
process used for logging should not modify the data stream, so the
encoding/decoding process should not be also migrating the data. Thus, we
update the class to the following:

public class Person {
  public var name: String
  @available(*, deprecated, message: "use 'phoneNumbers' instead")
  public var phoneNumber: String?
  public var phoneNumbers: [String]

  @available(*, deprecated, message: "use 'init(name:phoneNumbers:)' instead")
  public convenience init(name: String, phoneNumber: String?) {
    self.phoneNumber = phoneNumber
    self.init(name: name, phoneNumbers: [])
  }

  public init(name: String, phoneNumbers: [String] = []) {
    self.name = name
    self.phoneNumber = nil
    self.phoneNumbers = phoneNumbers
  }

  public convenience init?(from reader: Reader) {
    guard let name = reader.readString(withKey: "name") else {
      return nil
    }
    let phoneNumbers = reader.readStringArray(withKey: "phoneNumbers") ?? []
    self.phoneNumber = reader.readString(withKey: "phoneNumber")
    self.init(name: name, phoneNumbers: phoneNumbers)
  }

  public func write(to writer: Writer) {
    writer.write(name, key: "name")
    if let phoneNumber = phoneNumber {
      writer.write(phoneNumber, key: "phoneNumber")
    }
    if !phoneNumbers.isEmpty {
      writer.write(phoneNumbers, key: "phoneNumbers")
    }
  }
}

This type is backwards-compatible with the previous version and will warn
external users that they should migrate to the new version of the API; it
will also have its values preserved exactly if a middleman process parses
and then reserializes it.

The problem, however, is that the references to phoneNumber in Person's *own
methods* also emit deprecation warnings. This is undesirable because the
warnings cannot be easily avoided (see workarounds below). The crux of the
issue is that one of the two statements is likely to be true:

   1.

   At the time the user deprecates something, they also have the
   opportunity to migrate all references to it within the same file, thus
   eliminating the warnings there and making the file build cleanly.
   2.

   At the time the user deprecates something, they must continue to
   reference it internally to achieve correct and performant behavior, thus
   making it currently impossible to achieve a clean build for the file
   containing that declaration.

This proposal aims to solve the second issue.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#workarounds-and-their-problems>Workarounds
and their problems

One workaround would be to shadow deprecated declarations by making the
declaration itself private (and prepending something like an underscore to
the name) and then using a public passthrough property or method. The
private declaration would not be deprecated while the public one would be.

There are two main problems with this approach. First, it increases the
development and maintanence cost of the deprecation because the new
declaration has to be added and all internal references to the declaration
must be updated.

Secondly, and more importantly, this shadowing can have significant
performance penalties. Consider this example:

class DirectArrayHolder {
  @available(*, deprecated, message: "Use something else instead")
  var array: [String] = []
}
class IndirectArrayHolder {
  var _array: [String] = []

  @available(*, deprecated, message: "Use something else instead")
  var array: [String] {
    get { return _array }
    set { _array = newValue }
  }
}
func useDirectHolder() -> DirectArrayHolder {
  let holder = DirectArrayHolder()
  for _ in 0...10_000 {
    holder.array.append("foo")
  }
  return holder
}
func useIndirectHolder() -> IndirectArrayHolder {
  let holder = IndirectArrayHolder()
  for _ in 0...10_000 {
    holder.array.append("foo")
  }
  return holder
}

Behaviorally, these are the same. However, the generated code is different
because the memory management semantics change due to the passthrough
property. With full optimizations on, useDirectHolder executes in 1.360
sec while useIndirectHolder executes in 235.8 sec—over two orders of
magnitude slower!

It could be argued that this performance issue is something that could be
solved separately, but this should be considered just one motivating
example and not the sole motivation for this proposed change.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#proposed-solution>Proposed
solution

Do not print deprecation warnings for references to deprecated
declarations that are in the same file.

There would be no change for declarations marked unavailable. A
declaration marked unavailable is making a stronger statement about its
existence such that referencing it is an error; unlike marking a
declaration deprecated, marking it unavailable is a breaking change for
downstream clients, so the behavior of that attribute value should be
unchanged whether references are in the same file or elsewhere.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#implementation-details>Implementation
details

A small addition would be made to TypeChecker::diagnoseIfDeprecated so
that it returns early if the declaration is in the same source file as the
reference.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#impact-on-existing-code>Impact
on existing code

This is not a source-breaking change. The only side effect for existing
code is that deprecation warnings that were emitted for same-file
references in the past will no longer be emitted.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#alternatives-considered>Alternatives
considered

Omitting deprecation warnings for references in the same *module*, not
only the same *file*, was also considered. An argument could be made that
deprecation warnings are intended as messaging for external users of an
API, so drawing the line at the module level would be more appropriate than
at the file level. However, modules can be large and span a broad set of
APIs contributed by many developers on multi-person teams, and they may
want to use the @available attribute to phase out particular APIs for
internal use as well. Omitting deprecation warnings for anything in the
same module takes that useful tool away for those users.

Another idea that was brought up in a feature request
<https://bugs.swift.org/browse/SR-3357> filed for this was to allow a
"deprecated block" that would suppress all deprecation warnings inside it.
This would also solve the problem motivating this proposal and is similar
to the approach taken by other languages (such as Java's
@SuppressWarnings("deprecation") annotation), but this approach has
drawbacks as well. It would not distinguish between deprecation warnings in
the same file or module vs. those in other files or modules (unless
modifiers were added to it, which runs the risk of adding too much
complexity for too little gain). Furthermore, it has the potential to be
harmful if abused—developers could surround large code blocks with the
deprecation suppressor and then lose any deprecation warnings that would
have been emitted for APIs used inside that block.

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


(Tony Arnold) #4

Hi Tony,

These warnings are emitted for any reference to a deprecated entity, including those in the same file. In some cases, however, it may be necessary and correct to continue referring to the deprecated entity privately while discouraging its external use. In these scenarios, the warnings are superfluous and generate noise in the build logs. We propose eliminating these warnings for references made in the same file.

If same file is the direction that the proposal is leaning toward, what’s stopping you from creating an internal (or private) implementation that’s not deprecated. You could then call that via the public, deprecated method/property?

i.e.

@available(*, deprecated, message: "use 'phoneNumbers' instead")
public var phoneNumber: String? {
    get {
        return _phoneNumber
    }
    set {
        _phoneNumber = newValue
    }
}

private var _phoneNumber: String?

Am I missing something that would prevent this from working?

I can see that what you’re proposing would negate the need for the private backing property/method, but it seems like it’s introducing complexity for the few times you might need this.

thanks,

Tony

···

On 6 May 2017, at 04:12, Tony Allevato via swift-evolution <swift-evolution@swift.org> wrote:

----------
Tony Arnold
+61 411 268 532
http://thecocoabots.com/

ABN: 14831833541


(Zachary Waldowski) #5

I understand the reasoning and am in favor of it for those reasons, but
I will note that I use deprecations extremely frequently while
refactoring, and this would defeat that use case. As I have a few larger
projects with long compile times, losing that functionality would be
disappointing.
Sincerely,
  Zachary Waldowski
  zach@waldowski.me

On Fri, May 5, 2017, at 02:12 PM, Tony Allevato via swift-evolution wrote:> Hey Swift Evolvers,

I'd like to propose a change that would suppress deprecation warnings
when the reference to the deprecated declaration is in the same file
as that declaration.>
This personally affects one of my projects (Swift protocol buffers)
because .proto files allow declarations to be declared as deprecated,
and we'd like to surface that by deprecating the equivalent
declaration in the Swift code that we generate. We can't do this
without generating noisy build logs, because we still need to
reference the deprecated declarations to encode/decode the messages
correctly; and the workarounds have significant performance penalties
as described in the draft.>
I'd love some feedback from folks to know whether this—or something
like it—is something that seems desirable/undesirable for the
language. I've tinkered with an implementation in a branch[1] and it's
fairly straightforward.>
Gist link:
https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb>

Omit deprecation warnings for same-file references

* Proposal: SE-0000[2]
* Author(s): Tony Allevato[3]
* Status: Awaiting review[4]
* Review manager: TBD> Introduction

Public API developers can use the @available attribute in Swift to
mark APIs as deprecated, which will emit warnings when a user
references that API to notify them that that particular API is no
longer preferred or may disappear in the future.> These warnings are emitted for *any* reference to a deprecated
entity, including those in the same file. In some cases, however, it
may be necessary and correct to continue referring to the deprecated
entity privately while discouraging its external use. In these
scenarios, the warnings are superfluous and generate noise in the
build logs. We propose eliminating these warnings for references made
in the same file.> Swift-evolution thread: TBD[5]

Motivation

As APIs evolve, declarations may become deprecated but still need to
be referenced privately in order to function correctly. For example,
consider an object that is serialized/parsed between a client and
server. The first iteration of such a type might look like this (the
examples use a strawman API to avoid tying the discussion too closely
with the new coding APIs, which is not the focus):> public class Person { public var name: String public var
phoneNumber: String? public init(name: String, phoneNumber: String?
= nil) { self.name = name self.phoneNumber = phoneNumber } public
convenience init?(from reader: Reader) { guard let name =
reader.readString(withKey: "name") else { return nil } let
phoneNumber = reader.readString(withKey: "phoneNumber")
self.init(name: name, phoneNumber: phoneNumber) } public func
write(to writer: Writer) { writer.write(name, key: "name") if let
phoneNumber = phoneNumber { writer.write(phoneNumber, key:
"phoneNumber") } } }> Later, we decide that we need to support storing multiple phone
numbers for a Person. To avoid breaking source compatibility, we
deprecate the old property and add a new one. We still wish the
encoding/decoding process to preserve the integrity of the old data,
however—for example, a middleman process used for logging should not
modify the data stream, so the encoding/decoding process should not be
also migrating the data. Thus, we update the class to the following:> public class Person { public var name: String @available(*,
deprecated, message: "use 'phoneNumbers' instead") public var
phoneNumber: String? public var phoneNumbers: [String]
@available(*, deprecated, message: "use 'init(name:phoneNumbers:)'
instead") public convenience init(name: String, phoneNumber: String?)
{ self.phoneNumber = phoneNumber self.init(name: name, phoneNumbers:
[]) } public init(name: String, phoneNumbers: [String] = []) {
self.name = name self.phoneNumber = nil self.phoneNumbers =
phoneNumbers } public convenience init?(from reader: Reader) {
guard let name = reader.readString(withKey: "name") else { return nil
} let phoneNumbers = reader.readStringArray(withKey: "phoneNumbers")
?? [] self.phoneNumber = reader.readString(withKey: "phoneNumber")
self.init(name: name, phoneNumbers: phoneNumbers) } public func
write(to writer: Writer) { writer.write(name, key: "name") if let
phoneNumber = phoneNumber { writer.write(phoneNumber, key:
"phoneNumber") } if !phoneNumbers.isEmpty {
writer.write(phoneNumbers, key: "phoneNumbers") } } }> This type is backwards-compatible with the previous version and will
warn external users that they should migrate to the new version of the
API; it will also have its values preserved exactly if a middleman
process parses and then reserializes it.> The problem, however, is that the references to phoneNumber in
Person's *own methods* also emit deprecation warnings. This is
undesirable because the warnings cannot be easily avoided (see
workarounds below). The crux of the issue is that one of the two
statements is likely to be true:

1. At the time the user deprecates something, they also have the
    opportunity to migrate all references to it within the same file,
    thus eliminating the warnings there and making the file build
    cleanly.

2. At the time the user deprecates something, they must continue to
    reference it internally to achieve correct and performant
    behavior, thus making it currently impossible to achieve a clean
    build for the file containing that declaration.> This proposal aims to solve the second issue.

Workarounds and their problems

One workaround would be to shadow deprecated declarations by making
the declaration itself private (and prepending something like an
underscore to the name) and then using a public passthrough property
or method. The private declaration would not be deprecated while the
public one would be.> There are two main problems with this approach. First, it increases
the development and maintanence cost of the deprecation because the
new declaration has to be added and all internal references to the
declaration must be updated.> Secondly, and more importantly, this shadowing can have significant
performance penalties. Consider this example:> class DirectArrayHolder { @available(*, deprecated, message: "Use
something else instead") var array: [String] = [] }

class IndirectArrayHolder { var _array: [String] = [] @available(*,
deprecated, message: "Use something else instead") var array:
[String] { get { return _array } set { _array = newValue } } }

func useDirectHolder() -> DirectArrayHolder { let holder =
DirectArrayHolder() for _ in ...10_000 { holder.array.append("foo") }
return holder }

func useIndirectHolder() -> IndirectArrayHolder { let holder =
IndirectArrayHolder() for _ in ...10_000 { holder.array.append("foo")
} return holder }> Behaviorally, these are the same. However, the generated code is
different because the memory management semantics change due to the
passthrough property. With full optimizations on, useDirectHolder
executes in 1.360 sec while useIndirectHolder executes in 235.8
sec—over two orders of magnitude slower!> It could be argued that this performance issue is something that
could be solved separately, but this should be considered just
one motivating example and not the sole motivation for this
proposed change.> Proposed solution

Do not print deprecation warnings for references to deprecated
declarations that are in the same file.> There would be no change for declarations marked unavailable. A
declaration marked unavailable is making a stronger statement about
its existence such that referencing it is an error; unlike marking a
declaration deprecated, marking it unavailable is a breaking change
for downstream clients, so the behavior of that attribute value should
be unchanged whether references are in the same file or elsewhere.> Implementation details

A small addition would be made to TypeChecker::diagnoseIfDeprecated so
that it returns early if the declaration is in the same source file as
the reference.> Impact on existing code

This is not a source-breaking change. The only side effect for
existing code is that deprecation warnings that were emitted for same-
file references in the past will no longer be emitted.> Alternatives considered

Omitting deprecation warnings for references in the same *module*, not
only the same *file*, was also considered. An argument could be made
that deprecation warnings are intended as messaging for external users
of an API, so drawing the line at the module level would be more
appropriate than at the file level. However, modules can be large and
span a broad set of APIs contributed by many developers on multi-
person teams, and they may want to use the @available attribute to
phase out particular APIs for internal use as well. Omitting
deprecation warnings for anything in the same module takes that useful
tool away for those users.> Another idea that was brought up in a feature request[6] filed for
this was to allow a "deprecated block" that would suppress all
deprecation warnings inside it. This would also solve the problem
motivating this proposal and is similar to the approach taken by other
languages (such as Java's @SuppressWarnings("deprecation")
annotation), but this approach has drawbacks as well. It would not
distinguish between deprecation warnings in the same file or module
vs. those in other files or modules (unless modifiers were added to
it, which runs the risk of adding too much complexity for too little
gain). Furthermore, it has the potential to be harmful if
abused—developers could surround large code blocks with the
deprecation suppressor and then lose any deprecation warnings that
would have been emitted for APIs used inside that block.> _________________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

Links:

  1. https://github.com/apple/swift/compare/master...allevato:omit-deprecation-same-file
  2. https://github.com/apple/swift-evolution/blob/master/proposals/NNNN-name.md
  3. https://github.com/allevato
  4. https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#rationale
  5. http://thread.gmane.org/gmane.comp.lang.swift.evolution/
  6. https://bugs.swift.org/browse/SR-3357


(Xiaodi Wu) #6

Why guess as to which of these is appropriate? Couldn't you support the
current and all variants of this behavior by allowing access modifiers on
'deprecated'?

* public deprecated: warning when used from a different module, behaves as
though there's a public deprecated pass-through

* internal deprecated: warning when used from a different file

* fileprivate deprecated: warning when used from a different scope

* private deprecated: synonymous with deprecated for backwards
compatibility, behaves like it does today

(No need for complicated parsing; SE-25 allows a higher nominal access
modifier inside a lower one without warning, so it's fine to allow 'public
deprecated' to decorate a private member with no effect.)

···

On Fri, May 5, 2017 at 13:51 Tony Allevato via swift-evolution < swift-evolution@swift.org> wrote:

On Fri, May 5, 2017 at 11:37 AM BJ Homer <bjhomer@gmail.com> wrote:

I’ve run into this problem as well, when dealing with legacy file formats
(similar to the example in the proposal), and I agree it would be nice to
be able to address it. We’ve got a few “permanent” warnings in our code
base right now due to this. I’m not sure whether the deprecation should be
disabled for the entire file, though; perhaps it should be disabled just
within the type itself? I don’t know how that would contribute to the
complexity of the implementation, but it seems like we would still want to
warn about deprecated accesses between two types declared in the same file.

I initially picked "same file" because (1) it appears to be dead simple to
implement and (2) the idea that if someone is marking a definition as
private in a file, they can easily go through the same file and resolve any
uses of that declaration if they no longer want them. But it's possible
that there are helper types that just happen to be in the same file where
you'd like to be warned about using those "privately allowed APIs", so
you're right that same-file suppression would make those harder to detect.

Given that, I'd be open to either of these possibilities and I'd love to
hear what others think:

* Give deprecation-suppression "fileprivate" semantics
* Give deprecation-suppression "private" semantics (suppressed in the same
type and in extensions of that type within the same file)

-BJ Homer

On May 5, 2017, at 12:12 PM, Tony Allevato via swift-evolution < >> swift-evolution@swift.org> wrote:

Hey Swift Evolvers,

I'd like to propose a change that would suppress deprecation warnings
when the reference to the deprecated declaration is in the same file as
that declaration.

This personally affects one of my projects (Swift protocol buffers)
because .proto files allow declarations to be declared as deprecated, and
we'd like to surface that by deprecating the equivalent declaration in the
Swift code that we generate. We can't do this without generating noisy
build logs, because we still need to reference the deprecated declarations
to encode/decode the messages correctly; and the workarounds have
significant performance penalties as described in the draft.

I'd love some feedback from folks to know whether this—or something like
it—is something that seems desirable/undesirable for the language. I've
tinkered with an implementation in a branch
<https://github.com/apple/swift/compare/master...allevato:omit-deprecation-same-file>
and it's fairly straightforward.

Gist link:
https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb

Omit deprecation warnings for same-file references

   - Proposal: SE-0000
   <https://github.com/apple/swift-evolution/blob/master/proposals/NNNN-name.md>
   - Author(s): Tony Allevato <https://github.com/allevato>
   - Status: Awaiting review
   <https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#rationale>
   - Review manager: TBD

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#introduction>
Introduction

Public API developers can use the @available attribute in Swift to mark
APIs as deprecated, which will emit warnings when a user references that
API to notify them that that particular API is no longer preferred or may
disappear in the future.

These warnings are emitted for *any* reference to a deprecated entity,
including those in the same file. In some cases, however, it may be
necessary and correct to continue referring to the deprecated entity
privately while discouraging its external use. In these scenarios, the
warnings are superfluous and generate noise in the build logs. We propose
eliminating these warnings for references made in the same file.

Swift-evolution thread: TBD
<http://thread.gmane.org/gmane.comp.lang.swift.evolution/>

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#motivation>
Motivation

As APIs evolve, declarations may become deprecated but still need to be
referenced privately in order to function correctly. For example, consider
an object that is serialized/parsed between a client and server. The first
iteration of such a type might look like this (the examples use a strawman
API to avoid tying the discussion too closely with the new coding APIs,
which is not the focus):

public class Person {
  public var name: String
  public var phoneNumber: String?

  public init(name: String, phoneNumber: String? = nil) {
    self.name = name
    self.phoneNumber = phoneNumber
  }

  public convenience init?(from reader: Reader) {
    guard let name = reader.readString(withKey: "name") else {
      return nil
    }
    let phoneNumber = reader.readString(withKey: "phoneNumber")
    self.init(name: name, phoneNumber: phoneNumber)
  }

  public func write(to writer: Writer) {
    writer.write(name, key: "name")
    if let phoneNumber = phoneNumber {
      writer.write(phoneNumber, key: "phoneNumber")
    }
  }
}

Later, we decide that we need to support storing multiple phone numbers
for a Person. To avoid breaking source compatibility, we deprecate the
old property and add a new one. We still wish the encoding/decoding process
to preserve the integrity of the old data, however—for example, a middleman
process used for logging should not modify the data stream, so the
encoding/decoding process should not be also migrating the data. Thus, we
update the class to the following:

public class Person {
  public var name: String
  @available(*, deprecated, message: "use 'phoneNumbers' instead")
  public var phoneNumber: String?
  public var phoneNumbers: [String]

  @available(*, deprecated, message: "use 'init(name:phoneNumbers:)' instead")
  public convenience init(name: String, phoneNumber: String?) {
    self.phoneNumber = phoneNumber
    self.init(name: name, phoneNumbers: [])
  }

  public init(name: String, phoneNumbers: [String] = []) {
    self.name = name
    self.phoneNumber = nil
    self.phoneNumbers = phoneNumbers
  }

  public convenience init?(from reader: Reader) {
    guard let name = reader.readString(withKey: "name") else {
      return nil
    }
    let phoneNumbers = reader.readStringArray(withKey: "phoneNumbers") ?? []
    self.phoneNumber = reader.readString(withKey: "phoneNumber")
    self.init(name: name, phoneNumbers: phoneNumbers)
  }

  public func write(to writer: Writer) {
    writer.write(name, key: "name")
    if let phoneNumber = phoneNumber {
      writer.write(phoneNumber, key: "phoneNumber")
    }
    if !phoneNumbers.isEmpty {
      writer.write(phoneNumbers, key: "phoneNumbers")
    }
  }
}

This type is backwards-compatible with the previous version and will warn
external users that they should migrate to the new version of the API; it
will also have its values preserved exactly if a middleman process parses
and then reserializes it.

The problem, however, is that the references to phoneNumber in Person's *own
methods* also emit deprecation warnings. This is undesirable because the
warnings cannot be easily avoided (see workarounds below). The crux of the
issue is that one of the two statements is likely to be true:

   1.

   At the time the user deprecates something, they also have the
   opportunity to migrate all references to it within the same file, thus
   eliminating the warnings there and making the file build cleanly.
   2.

   At the time the user deprecates something, they must continue to
   reference it internally to achieve correct and performant behavior, thus
   making it currently impossible to achieve a clean build for the file
   containing that declaration.

This proposal aims to solve the second issue.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#workarounds-and-their-problems>Workarounds
and their problems

One workaround would be to shadow deprecated declarations by making the
declaration itself private (and prepending something like an underscore to
the name) and then using a public passthrough property or method. The
private declaration would not be deprecated while the public one would be.

There are two main problems with this approach. First, it increases the
development and maintanence cost of the deprecation because the new
declaration has to be added and all internal references to the declaration
must be updated.

Secondly, and more importantly, this shadowing can have significant
performance penalties. Consider this example:

class DirectArrayHolder {
  @available(*, deprecated, message: "Use something else instead")
  var array: [String] = []
}
class IndirectArrayHolder {
  var _array: [String] = []

  @available(*, deprecated, message: "Use something else instead")
  var array: [String] {
    get { return _array }
    set { _array = newValue }
  }
}
func useDirectHolder() -> DirectArrayHolder {
  let holder = DirectArrayHolder()
  for _ in 0...10_000 {
    holder.array.append("foo")
  }
  return holder
}
func useIndirectHolder() -> IndirectArrayHolder {
  let holder = IndirectArrayHolder()
  for _ in 0...10_000 {
    holder.array.append("foo")
  }
  return holder
}

Behaviorally, these are the same. However, the generated code is
different because the memory management semantics change due to the
passthrough property. With full optimizations on, useDirectHolder executes
in 1.360 sec while useIndirectHolder executes in 235.8 sec—over two
orders of magnitude slower!

It could be argued that this performance issue is something that could be
solved separately, but this should be considered just one motivating
example and not the sole motivation for this proposed change.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#proposed-solution>Proposed
solution

Do not print deprecation warnings for references to deprecated
declarations that are in the same file.

There would be no change for declarations marked unavailable. A
declaration marked unavailable is making a stronger statement about its
existence such that referencing it is an error; unlike marking a
declaration deprecated, marking it unavailable is a breaking change for
downstream clients, so the behavior of that attribute value should be
unchanged whether references are in the same file or elsewhere.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#implementation-details>Implementation
details

A small addition would be made to TypeChecker::diagnoseIfDeprecated so
that it returns early if the declaration is in the same source file as the
reference.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#impact-on-existing-code>Impact
on existing code

This is not a source-breaking change. The only side effect for existing
code is that deprecation warnings that were emitted for same-file
references in the past will no longer be emitted.

<https://gist.github.com/allevato/76f098a761147f3be5017d10a6f4ecbb#alternatives-considered>Alternatives
considered

Omitting deprecation warnings for references in the same *module*, not
only the same *file*, was also considered. An argument could be made
that deprecation warnings are intended as messaging for external users
of an API, so drawing the line at the module level would be more
appropriate than at the file level. However, modules can be large and span
a broad set of APIs contributed by many developers on multi-person teams,
and they may want to use the @available attribute to phase out
particular APIs for internal use as well. Omitting deprecation warnings
for anything in the same module takes that useful tool away for those users.

Another idea that was brought up in a feature request
<https://bugs.swift.org/browse/SR-3357> filed for this was to allow a
"deprecated block" that would suppress all deprecation warnings inside it.
This would also solve the problem motivating this proposal and is similar
to the approach taken by other languages (such as Java's
@SuppressWarnings("deprecation") annotation), but this approach has
drawbacks as well. It would not distinguish between deprecation warnings in
the same file or module vs. those in other files or modules (unless
modifiers were added to it, which runs the risk of adding too much
complexity for too little gain). Furthermore, it has the potential to be
harmful if abused—developers could surround large code blocks with the
deprecation suppressor and then lose any deprecation warnings that would
have been emitted for APIs used inside that block.

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

_______________________________________________

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


(Tony Allevato) #7

Hi Tony,

>
> These warnings are emitted for any reference to a deprecated entity,
including those in the same file. In some cases, however, it may be
necessary and correct to continue referring to the deprecated entity
privately while discouraging its external use. In these scenarios, the
warnings are superfluous and generate noise in the build logs. We propose
eliminating these warnings for references made in the same file.

If same file is the direction that the proposal is leaning toward, what’s
stopping you from creating an internal (or private) implementation that’s
not deprecated. You could then call that via the public, deprecated
method/property?

Hi Tony, this is called out in the proposal :slight_smile: In addition to the fact that
it requires writing boilerplate code manually and updating all references
to the deprecated property in type's implementation, there are performance
implications as well due to the change in memory management behavior. In
the example presented in the proposal:

Behaviorally, these are the same. However, the generated code is different
because the memory management semantics change due to the passthrough
property. With full optimizations on, useDirectHolder executes in 1.360 sec
while useIndirectHolder executes in 235.8 sec—over two orders of magnitude
slower!

···

On Fri, May 5, 2017 at 3:20 PM Tony Arnold <tony@thecocoabots.com> wrote:

> On 6 May 2017, at 04:12, Tony Allevato via swift-evolution < > swift-evolution@swift.org> wrote:

i.e.

@available(*, deprecated, message: "use 'phoneNumbers' instead")
public var phoneNumber: String? {
    get {
        return _phoneNumber
    }
    set {
        _phoneNumber = newValue
    }
}

private var _phoneNumber: String?

Am I missing something that would prevent this from working?

I can see that what you’re proposing would negate the need for the private
backing property/method, but it seems like it’s introducing complexity for
the few times you might need this.

thanks,

Tony

----------
Tony Arnold
+61 411 268 532 <+61%20411%20268%20532>
http://thecocoabots.com/

ABN: 14831833541


(Brent Royal-Gordon) #8

[Re-sending with minor edits since this didn't initially go to the list.]

···

On May 5, 2017, at 3:25 PM, Tony Allevato via swift-evolution <swift-evolution@swift.org> wrote:

Hi Tony, this is called out in the proposal :slight_smile: In addition to the fact that it requires writing boilerplate code manually and updating all references to the deprecated property in type's implementation, there are performance implications as well due to the change in memory management behavior.

To be blunt: Why should we care if a deprecated symbol is slow? It's deprecated, so you shouldn't be using it. (And it's only the public-facing part of it that's slow, which you *really* shouldn't be using.)

Besides, I have a hard time believing we couldn't detect when the getter is just `return otherProperty` and the setter is just `otherProperty = newValue` and optimize it better.

Sent from my iPad


(BJ Homer) #9

I’m not opposed to more configurability like that. I worry it makes the feature more complicated and potentially delays the acceptance or implementation of this feature, though. If it’s easy to implement, though, then sure, I like that.

-BJ

···

On May 5, 2017, at 1:34 PM, Xiaodi Wu <xiaodi.wu@gmail.com> wrote:

Why guess as to which of these is appropriate? Couldn't you support the current and all variants of this behavior by allowing access modifiers on 'deprecated'?

* public deprecated: warning when used from a different module, behaves as though there's a public deprecated pass-through

* internal deprecated: warning when used from a different file

* fileprivate deprecated: warning when used from a different scope

* private deprecated: synonymous with deprecated for backwards compatibility, behaves like it does today

(No need for complicated parsing; SE-25 allows a higher nominal access modifier inside a lower one without warning, so it's fine to allow 'public deprecated' to decorate a private member with no effect.)


(Tony Allevato) #10

I'm inclined to agree. I'm not opposed outright to that degree of
configurability but at the same time I wonder if the complexity is
needed—it feels like it's getting close to the "fine-tuned auditing" that I
argued against during the discussions about access control.

It could also be done additively later, if a significant amount of people
using the feature found that they did need it.

···

On Fri, May 5, 2017 at 3:09 PM BJ Homer <bjhomer@gmail.com> wrote:

> On May 5, 2017, at 1:34 PM, Xiaodi Wu <xiaodi.wu@gmail.com> wrote:
>
> Why guess as to which of these is appropriate? Couldn't you support the
current and all variants of this behavior by allowing access modifiers on
'deprecated'?
>
> * public deprecated: warning when used from a different module, behaves
as though there's a public deprecated pass-through
>
> * internal deprecated: warning when used from a different file
>
> * fileprivate deprecated: warning when used from a different scope
>
> * private deprecated: synonymous with deprecated for backwards
compatibility, behaves like it does today
>
> (No need for complicated parsing; SE-25 allows a higher nominal access
modifier inside a lower one without warning, so it's fine to allow 'public
deprecated' to decorate a private member with no effect.)

I’m not opposed to more configurability like that. I worry it makes the
feature more complicated and potentially delays the acceptance or
implementation of this feature, though. If it’s easy to implement, though,
then sure, I like that.

-BJ


(Tony Allevato) #11

[Re-sending with minor edits since this didn't initially go to the list.]

>
> Hi Tony, this is called out in the proposal :slight_smile: In addition to the fact
that it requires writing boilerplate code manually and updating all
references to the deprecated property in type's implementation, there are
performance implications as well due to the change in memory management
behavior.

(Sorry that I didn't forward this myself—I was off-the-grid over the
weekend.)

To be blunt: Why should we care if a deprecated symbol is slow? It's

deprecated, so you shouldn't be using it. (And it's only the public-facing
part of it that's slow, which you *really* shouldn't be using.)

Imagine you're a client of version X of a library. You see that X+1 has
been released and has some bug fixes you would benefit from, so you
upgrade. But, the library author has also deprecated some APIs you use and
done the property-shadowing trick to get around it internally. Now you
either have the choice of (1) taking a big performance penalty or (2)
migrating immediately to replace usage of the deprecated API. Depending on
release pressures and other real-world engineering factors, option 2 may
not be feasible—maybe the migration path is non-trivial—and you'd rather
just deal with some warnings until you have the opportunity to migrate
properly.

Deprecation and performance are completely unrelated concerns, so I don't
think we should be satisfied by the answer "you're not supposed to use it
so performance doesn't matter". The workaround being discussed is an action
taken by the library author to help him/herself (and not even that much of
a help, because it requires cleaning up all the internal references to
avoid deprecation warnings there) but unconditionally penalizes the clients
of that library.

Besides, I have a hard time believing we couldn't detect when the getter
is just `return otherProperty` and the setter is just `otherProperty =
newValue` and optimize it better.

I totally agree it would be nice if this was optimized. I don't have a deep
enough understanding of Swift's memory management logic to know why the
behavior is what it is today. (Looking at the optimized assembly code, it
doesn't seem to actually call the getter/setter, but the underlying
alloc/retain/release calls that get generated are slightly different in
each case.)

···

On Sat, May 6, 2017 at 9:21 PM Brent Royal-Gordon <brent@architechies.com> wrote:

> On May 5, 2017, at 3:25 PM, Tony Allevato via swift-evolution < > swift-evolution@swift.org> wrote:

Sent from my iPad


(Xiaodi Wu) #12

The reason I suggest this is that the most consistent boundary with the
rest of Swift is the module. I don't doubt that some may wish to deprecate
features even for some subset of internal users, but it seems plainly
obvious to me that the more common scenario will be deprecating for public
consumption only.

Since you're presenting scenarios in which it might make sense to have
scope-based or file-based deprecation, then more fine tuning would be the
answer. Without that, I'd argue the default ought to be what I call "public
deprecated." The within-a-module use case should be the later addition.

···

On Fri, May 5, 2017 at 17:15 Tony Allevato <tony.allevato@gmail.com> wrote:

I'm inclined to agree. I'm not opposed outright to that degree of
configurability but at the same time I wonder if the complexity is
needed—it feels like it's getting close to the "fine-tuned auditing" that I
argued against during the discussions about access control.

It could also be done additively later, if a significant amount of people
using the feature found that they did need it.

On Fri, May 5, 2017 at 3:09 PM BJ Homer <bjhomer@gmail.com> wrote:

> On May 5, 2017, at 1:34 PM, Xiaodi Wu <xiaodi.wu@gmail.com> wrote:
>
> Why guess as to which of these is appropriate? Couldn't you support the
current and all variants of this behavior by allowing access modifiers on
'deprecated'?
>
> * public deprecated: warning when used from a different module, behaves
as though there's a public deprecated pass-through
>
> * internal deprecated: warning when used from a different file
>
> * fileprivate deprecated: warning when used from a different scope
>
> * private deprecated: synonymous with deprecated for backwards
compatibility, behaves like it does today
>
> (No need for complicated parsing; SE-25 allows a higher nominal access
modifier inside a lower one without warning, so it's fine to allow 'public
deprecated' to decorate a private member with no effect.)

I’m not opposed to more configurability like that. I worry it makes the
feature more complicated and potentially delays the acceptance or
implementation of this feature, though. If it’s easy to implement, though,
then sure, I like that.

-BJ


#13

One potential pitfall of auto-disabling the warning for various scopes, especially the file one, is that one can by mistake add a new unintended reference. Even though it require extra change when deprecating, should there be instead a way to tag the "authorized" use.

@useDeprecated(self.phoneNumber) = phoneNumber

Not sure if something like that is possible, though it could make the code ugly, and may goes against the unwritten rule that some have: do not alter the deprecated code section itself. Maybe a similar marker for the function itself:

@usingDeprecated(Phone.phoneNumber)
@available(*, deprecated, ...)
public convenience init( ... ) { ... }

Dany

···

Le 5 mai 2017 à 18:15, Tony Allevato via swift-evolution <swift-evolution@swift.org> a écrit :

I'm inclined to agree. I'm not opposed outright to that degree of configurability but at the same time I wonder if the complexity is needed—it feels like it's getting close to the "fine-tuned auditing" that I argued against during the discussions about access control.

It could also be done additively later, if a significant amount of people using the feature found that they did need it.

On Fri, May 5, 2017 at 3:09 PM BJ Homer <bjhomer@gmail.com> wrote:

> On May 5, 2017, at 1:34 PM, Xiaodi Wu <xiaodi.wu@gmail.com> wrote:
>
> Why guess as to which of these is appropriate? Couldn't you support the current and all variants of this behavior by allowing access modifiers on 'deprecated'?
>
> * public deprecated: warning when used from a different module, behaves as though there's a public deprecated pass-through
>
> * internal deprecated: warning when used from a different file
>
> * fileprivate deprecated: warning when used from a different scope
>
> * private deprecated: synonymous with deprecated for backwards compatibility, behaves like it does today
>
> (No need for complicated parsing; SE-25 allows a higher nominal access modifier inside a lower one without warning, so it's fine to allow 'public deprecated' to decorate a private member with no effect.)

I’m not opposed to more configurability like that. I worry it makes the feature more complicated and potentially delays the acceptance or implementation of this feature, though. If it’s easy to implement, though, then sure, I like that.

-BJ

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


(Anders) #14

The problem happened on deprecated public protocols in ReactiveSwift. Since conformance cannot be stripped until it is inaccessible, we have a bunch of deprecation warnings on conformance clauses, scattered in the codebase. Having this on a file basis is not enough in our cases, while for sure not all deprecations need such functionality.

So IMO, an opt-in annotation for external-only depreciations is enough to cover all practical cases without over-engineering or unanticipated shadowing.

Regards
Anders

···

On 6 May 2017, at 07:00, Xiaodi Wu via swift-evolution <swift-evolution@swift.org> wrote:

The reason I suggest this is that the most consistent boundary with the rest of Swift is the module. I don't doubt that some may wish to deprecate features even for some subset of internal users, but it seems plainly obvious to me that the more common scenario will be deprecating for public consumption only.

Since you're presenting scenarios in which it might make sense to have scope-based or file-based deprecation, then more fine tuning would be the answer. Without that, I'd argue the default ought to be what I call "public deprecated." The within-a-module use case should be the later addition.

On Fri, May 5, 2017 at 17:15 Tony Allevato <tony.allevato@gmail.com> wrote:
I'm inclined to agree. I'm not opposed outright to that degree of configurability but at the same time I wonder if the complexity is needed—it feels like it's getting close to the "fine-tuned auditing" that I argued against during the discussions about access control.

It could also be done additively later, if a significant amount of people using the feature found that they did need it.

On Fri, May 5, 2017 at 3:09 PM BJ Homer <bjhomer@gmail.com> wrote:

> On May 5, 2017, at 1:34 PM, Xiaodi Wu <xiaodi.wu@gmail.com> wrote:
>
> Why guess as to which of these is appropriate? Couldn't you support the current and all variants of this behavior by allowing access modifiers on 'deprecated'?
>
> * public deprecated: warning when used from a different module, behaves as though there's a public deprecated pass-through
>
> * internal deprecated: warning when used from a different file
>
> * fileprivate deprecated: warning when used from a different scope
>
> * private deprecated: synonymous with deprecated for backwards compatibility, behaves like it does today
>
> (No need for complicated parsing; SE-25 allows a higher nominal access modifier inside a lower one without warning, so it's fine to allow 'public deprecated' to decorate a private member with no effect.)

I’m not opposed to more configurability like that. I worry it makes the feature more complicated and potentially delays the acceptance or implementation of this feature, though. If it’s easy to implement, though, then sure, I like that.

-BJ

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


(Tony Allevato) #15

The reason I suggest this is that the most consistent boundary with the
rest of Swift is the module. I don't doubt that some may wish to deprecate
features even for some subset of internal users, but it seems plainly
obvious to me that the more common scenario will be deprecating for public
consumption only.

Normally I'd agree with this—I tend to think caring too much about access
control beyond internal/external API boundaries is a bit too fine-grained.
But...

Since you're presenting scenarios in which it might make sense to have

scope-based or file-based deprecation, then more fine tuning would be the
answer. Without that, I'd argue the default ought to be what I call "public
deprecated." The within-a-module use case should be the later addition.

After seeing some of the considerations from other folks who replied in the
thread, I've come around to your suggestion that more fine-tuning is the
best approach moving forward.

The case for deprecate-externally-but-not-internally was made well (a
library has to maintain conformances to/constraints against deprecated
protocols throughout their module and wants to do so without build noise,
even if the protocols are deprecated externally). Likewise,
deprecated-everywhere as a refactoring tool can be a boon to developer
productivity. So, thanks to those who replied with those use cases—that's
the kind of feedback I was looking for. :slight_smile:

Protocol buffers (and more generally, many code-generation use cases) pose
a unique kind of problem where deprecated-outside-of-the-file/scope is
valuable. Deprecate-externally isn't helpful here because of the nature of
how Swift modules and Xcode play together—until Xcode supports static
libraries with Swift code, users who want to package something like
protobufs are going to generate that code into the same module as the rest
of their app's code. This would make deprecated-externally unable to filter
out the warnings that I want to ignore inside the generated code itself.

So I'm happy to update my draft to define how this would look based on your
strawman syntax, but I imagine that it's now getting past the point of
"easy to implement by me or someone else in the Swift 4 timeframe" so I'll
put it on the back burner for the time being.

···

On Fri, May 5, 2017 at 4:00 PM Xiaodi Wu <xiaodi.wu@gmail.com> wrote:

On Fri, May 5, 2017 at 17:15 Tony Allevato <tony.allevato@gmail.com> > wrote:

I'm inclined to agree. I'm not opposed outright to that degree of
configurability but at the same time I wonder if the complexity is
needed—it feels like it's getting close to the "fine-tuned auditing" that I
argued against during the discussions about access control.

It could also be done additively later, if a significant amount of people
using the feature found that they did need it.

On Fri, May 5, 2017 at 3:09 PM BJ Homer <bjhomer@gmail.com> wrote:

> On May 5, 2017, at 1:34 PM, Xiaodi Wu <xiaodi.wu@gmail.com> wrote:
>
> Why guess as to which of these is appropriate? Couldn't you support
the current and all variants of this behavior by allowing access modifiers
on 'deprecated'?
>
> * public deprecated: warning when used from a different module,
behaves as though there's a public deprecated pass-through
>
> * internal deprecated: warning when used from a different file
>
> * fileprivate deprecated: warning when used from a different scope
>
> * private deprecated: synonymous with deprecated for backwards
compatibility, behaves like it does today
>
> (No need for complicated parsing; SE-25 allows a higher nominal access
modifier inside a lower one without warning, so it's fine to allow 'public
deprecated' to decorate a private member with no effect.)

I’m not opposed to more configurability like that. I worry it makes the
feature more complicated and potentially delays the acceptance or
implementation of this feature, though. If it’s easy to implement, though,
then sure, I like that.

-BJ


(Víctor Pimentel) #16

The reason I suggest this is that the most consistent boundary with the
rest of Swift is the module. I don't doubt that some may wish to deprecate
features even for some subset of internal users, but it seems plainly
obvious to me that the more common scenario will be deprecating for public
consumption only.

Normally I'd agree with this—I tend to think caring too much about access
control beyond internal/external API boundaries is a bit too fine-grained.
But...

Since you're presenting scenarios in which it might make sense to have

scope-based or file-based deprecation, then more fine tuning would be the
answer. Without that, I'd argue the default ought to be what I call "public
deprecated." The within-a-module use case should be the later addition.

After seeing some of the considerations from other folks who replied in
the thread, I've come around to your suggestion that more fine-tuning is
the best approach moving forward.

I think that different needs for that warning suppression would arise
inside the community. Some folks may want deprecated things inside a module
to trigger warnings (for example in a monolithic app) while others may not.

The best option for me is stated in the "alternatives considered" section
of your proposal: just adding a "suppress warnings block" like most other
languages (Java, C/ObjC, even Swiftlint). That way everyone can apply to
their use cases as needed and scales to every case.

About the potential of abuse, in my recent experience with ObjC, we have a
policy of zero warnings and we hardly ever use those suppression warnings.
Of course, like with every other feature, it can be abused, but it's not
difficult to keep it controlled.

From my point of view this will be the only way of having reasonable

defaults for not only this warning but for every other warning. Otherwise
we end up in a place where the rules for each warning are so intricate that
nobody understands them.

···

On Tue, May 9, 2017 at 12:34 AM, Tony Allevato via swift-evolution < swift-evolution@swift.org> wrote:

On Fri, May 5, 2017 at 4:00 PM Xiaodi Wu <xiaodi.wu@gmail.com> wrote:

--

Víctor Pimentel