As SE-0443 has been accepted and the initial implementation has been landed on main (thanks to @Douglas_Gregor) I'm starting to add new diagnostic groups. This thread is dedicated to discussing the deprecated diagnostic group and its subgroups.
At this iteration I'm adding the following groups:
When I think generally about the problem "I want to tweak the severity of deprecation warnings", that's usually solely limited to warnings about the availability of APIs (so, availability_deprecated that you mentioned at the end).
On the other hand, many of these feel like a disparate set of language features that have over time become disfavored. Some of them are even for features yet to be released, like attr_objcImplementation_deprecated (deprecating the old unsupported spelling @_objcImplementation for the new @implementation feature).
From a quick glance, I'm not sure there's much that really binds these together aside from the fact that _deprecated is in the name. A couple of them are related to each other (e.g., the two property_wrapper_* IDs), but I think most of these are really just groups of one.
Not strictly related but since it's mentioned here: attr_implementationOnly_deprecated should be removed entirely. internal import is not equivalent to @_implementationOnly (the former does not do dependency pruning for non-resilient modules) so there are situations where legitimate use cases now have this unsilenceable warning.
EDIT: I suppose there is one grouping that would be useful: the one containing all of those warnings, as a grouping of "deprecated language features". Then, I could say that I want to upgrade all of those to errors without also upgrading API availability deprecations. (And further, I could then downgrade specific warnings from that set above if I needed to for individual modules.)
Yes, I agree with Tony that we should not conflate library deprecations and language feature deprecations. If we need groups to cover them, I think it's probable that each language feature should be given its own group.
Is underscore_separated_lowercase the right convention for this? I don't think we use that style for literally anything else user-facing in Swift. I would expect this to use the same TitleCase style as feature names.
Do you mean the diagnostic group or the diagnostic itself?
It's been so much work to move to 5.9 because of this warning so I definitely want to have an option for it.
I have to point out that availability_deprecated is not the only group that is used for these kinds of deprecation. There are also witness_deprecated and conformance_availability_deprecated.
What if we make a group that includes API-deprecation-related groups (api_deprecated) and a group for deprecation of the language features (language_feature_deprecated)?
If we don't want a group so broad that it includes all the deprecation warnings, I'd still name a group for API deprecations more specifically like api_deprecated, and reserve the name deprecated for future use.
I don't have a strong opinion on this, and we have some time for renaming until we release 6.1. TitleCase is fine with me, so unless there's a contrary opinion, I can rename everything.
Bike-shedding the name, I'd prefer we don't use "API" here. Deprecation can happen to any declaration, whether it is a public declaration in another module, or a private declaration in your own module which I don't think people would think of as API. I think something like deprecated_declaration (or DeprecatedDeclaration) would be more accurate.
Regarding language feature deprecation, I think it would be too broad to control with a single group. Different language feature deprecations have different impacts and levels of urgency, so I would expect developers to potentially want to control it by feature. I think following a pattern of DeprecatedThing where Thing is the language feature name would be best.
The diagnostic itself. I don't want to derail further; I would just hesitate to do anything more with this one because I think it shouldn't be present at all until the feature it's warning about has been completely replaced.
Yeah, I agree, and I think in most cases sticking to the narrower groups should be recommended. But generally speaking, the existence of a broader group doesn't limit you from using a narrower group. So the question is whether we want to provide only narrower groups for each language feature individually or in addition to them also a broader group that includes them.
From the discussion so far I am inclined not to provide the broader group, and leave this decision for later. And as a consequence rename the deprecated group to your suggestion DeprecatedDeclaration.
I'm renaming the groups and struggling to find a good name for availability_deprecated.
Here is a quick recap of the groups included in the DeprecatedDeclaration group:
DeprecatedWitness - when a protocol requirement has a default implementation marked as deprecated
protocol P {
func f()
}
extension P {
@available(*, deprecated)
func f() {}
}
struct S: P {} // deprecated default implementation is used to satisfy instance method 'f()' required by protocol 'P'
DeprecatedConformance - when a type's conformance to a protocol is marked as deprecated
struct S {}
protocol P {}
@available(*, deprecated)
extension S: P {}
func f(_ p: some P) {}
f(S()) // Conformance of 'S' to 'P' is deprecated
availability_deprecated - when a protocol/type/function/... is marked as deprecated
To me, all of these diagnostics could just be covered under a single DeprecatedDeclaration group, no sub-groups needed. What motivates the need for distinct control over these three diagnostics?
To elaborate a bit on my objection to this breakdown, I worry that giving these diagnostics distinct diagnostic groups would be conflating the compiler's implementation of the diagnostics and the language meaning of the diagnostics. Use of deprecated conformances, use of deprecated default implementations, and general use of deprecated declarations are all diagnosing the same concept, IMO. However, they need distinct diagnostic implementations because there isn't a convenient way to implement or present the diagnostic using a single piece of code and a single templated string.
DeprecatedDeclaration looks super good. It makes sense both in how it's named and as a group.
For the others, it would be helpful if you could share what the corresponding diagnostic text currently is for these. The reason is that there's a good number of these for which the proposed group name (regardless of how consistent the capitalization is) is both long and essentially meaningless or possibly misleading to the end user. For example, what does "outer access" mean? How can a proper wrapper initalizer's initial value be altogether deprecated? And so on... It's worth making these names actually intelligible if they're going to be seven or eight words long; otherwise, they might as well be some arbitrary identifier such as "W0042a."
For a number of these which are immediately understandable, I think the "Deprecated" part can be dropped as superfluous for the end user in the context of a warning. For example, the warning group can just be TupleShuffleReordering. The user who cares if this diagnostic is a warning or error likely already knows—and if they don't, doesn't have to care—that the reason there's a diagnostic is that it's a deprecated feature: that's just historical exposition.
There's also leakage of some internal abbreviations such as "Decl" and "Attr" which, given the precedent of DeprecatedDeclaration (not DeprecatedDecl), can be spelled out for the end user. Combining this point with the prior paragraph's, for instance, I'd suggest ImplementationOnlyAttribute for the group name: that using the attribute causes a warning because it's deprecated is sort of beside the point if a user actively wants to upgrade or downgrade the warning.
On a cursory examination of some of the specific warnings, DeprecatedExecutorEnqueueOwnedJobImplementation and DeprecatedHashValueImplementation appear to be custom warnings for specific deprecated protocol requirements—which probably means that they could go under the umbrella of DeprecatedDeclaration? I'd imagine that there are some others of that ilk too.
I wrote documentation for each of the groups. It lives in userdocs/diagnostic_groups/ (link). Basically, the names are chosen to be rather long to not clash and be searchable. I'm open to any further suggestions on the naming.
One thing in favor of the Deprecated supergroup: In the diagnostic engine, we have an enum called DiagnosticOptions that has a case Deprecation. When a diagnostic has this option the diagnostic engine associates the deprecation category with it.
With the Deprecated group we could remove this option and check whether a diagnostic belongs to this group instead.
I do want to push back somewhat on this approach. Besides affecting readability to the human user, "rather long" names can cause other problems, e.g., with Windows command line limits.
If the overriding priority is not clashing and searchability, then a serial number like "W0024a" would fulfill those criteria completely; I like that we are trying to actually describe diagnostics in a human intelligible way, so I think it's worth making that a priority and don't think that preemptively "salting" these names does us any favors.
I wonder about using Deprecated as a common prefix. Prefer a suffix?
As a prefix it turns the whole into a noun, and it defers the different part making it much harder to scan for differences.
The suffix form like ImplicitlyUnwrappedOptionalDeprecated reads like a sentence with the implicit copula (x is deprecated). It avoids mixing the normative qualifier (deprecated) with the specifier (implicitly-unwrapped).
Most user are coming bottom-up from a specific feature, and only thinking top-down/group-wise when having to specify a number of them.
Using a suffix could also make it easier to have different kinds of deprecations (availability v. language) without muddying the namespace too much.
If prefixing is preferred, perhaps a briefer prefix: like legacy API prefixes DP, or some informal alias like Old or some injunction like Avoid. It might help for warnings/errors to read like a brief, normative statement or injunction.
On that last point, I would really hope that the Windows compiler has full support for response files as the Apple and Linux builds do, and folks running into command line length issues could move the arguments to a separate file if needed.
Two additional related suggestions with respect to process:
(a) While I totally agree with the overarching goal to classify all warnings—and certainly all new warnings going forward—I think users will be so overjoyed to be able to upgrade/downgrade DeprecatedDeclaration that the utility of holding it up even a little bit to settle the naming for DeprecatedInvalidRedeclSwift5 and the like is questionable if not negative.
(b) While it's wonderful that we still support Swift 4 mode and that new features such as this can be used in Swift 4 mode, specifically naming warnings that have already been upgraded to errors since Swift 5 such as DeprecatedInvalidRedeclSwift5 should probably be deprioritized if not altogether infinitely postponed. I doubt there's a single person who's ever going to toggle this: they're deprecation warnings that are themselves deprecated.