[swift-evolution] Pitch: Omit deprecation warnings for same-file references

[Tony Allevato]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20170505/98de009b/attachment.html>