[swift-evolution] Pitch: Omit deprecation warnings for same-file references
Tony Allevato
tony.allevato at gmail.com
Fri May 5 13:51:43 CDT 2017
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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20170505/a02c74cf/attachment.html>
More information about the swift-evolution
mailing list