[swift-evolution] Pitch: Omit deprecation warnings for same-file references
Xiaodi Wu
xiaodi.wu at gmail.com
Fri May 5 14:34:17 CDT 2017
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 at swift.org> wrote:
> On Fri, May 5, 2017 at 11:37 AM BJ Homer <bjhomer at 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 at 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 at swift.org
>> https://lists.swift.org/mailman/listinfo/swift-evolution
>>
>>
>> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org
> https://lists.swift.org/mailman/listinfo/swift-evolution
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20170505/baf4c6e7/attachment-0001.html>
More information about the swift-evolution
mailing list