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 > <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
_______________________________________________ swift-evolution mailing list swift-evolution@swift.org https://lists.swift.org/mailman/listinfo/swift-evolution
