deprecatedError + restricted annotation

[eed3si9n]

This implements restricted annotation as a generic form of deprecated annotation. restricted takes label and defaultSeverity parameter.

The user can configure how the usages of restricted API are handled by passing in -Wconfig config expressions.

A warning config expression may be <label>, <label>:<severity>, or <label>:<prefix>:<severity>

<label> is a String passed into @restricted.
<severity> must be one of "error", "warning", "info", or "none": default: warning.
<prefix> denotes the prefix of the restricted API. For example, "foo.*" will denote
that this config would apply only to calls under foo package/object.
Note: "*" is simply ignored.

The following configuration would escalate apiMayChange restriction to an error:

-Wconfig apiMayChange:foo.*:error

[dwijnand]

This is great!

Just a point of admin: this isn't backwards-compatible with 2.13.0, so this would have to land in either 2.13.0-RC1 or slip to 2.14.

[som-snytt]

There's a WIP of Adriaan's deprescalator. It makes deprecations go up, down, or silent, by use site or deprecatee.

I haven't fully digested the feature requests on the ticket for ad-hoc @deprecation, or maybe it's ad-hoc @migration.

[nafg]

Isn't it redundant with @compileTimeOnly? I guess this one wouldn't let a macro remove it, but is that necessary?

[eed3si9n]

@nafg @compileTimeError and @compileTimeOnly are similar from the point of view of compiler implementation, but the intents are different. The "only" in compileTimeOnly says it admits the method up until type checking, but compileTimeError is saying no the whole time.
IMO it's useful to have this as a separate construct, so for example tools can potentially do special things (not include in Scaladocs, tab completion etc) with the restligeist methods.

[som-snytt]

I've got a PR almost ready for @compileTimeEspecially, for type params which we especially expect to confer compile-time knowledge and benefits. It will also bump -Winfer-any, like -Werror:infer-any.

def f[@compileTimeEspecially("I better know what I'm doing") A](x: Any) =
  x match { case _: A => }

OK, that started out as a joke.

[adriaanm]

I would like to generalize the name to subsume internalOnly, apiMayChange, deprecated, poorPerformance,....

How about restricted(message: String, label: String, severity: Int)?

we could configure according to label and severity for a myriad ways of telling the user off

[dwijnand]

That assumes those all naturally fit in an int axis, which I don't think is true. I think it's plausible to imagine some users wanting to see those poorPerformance warnings but being fine with the apiMayChange ones, and some other users wanting the inverse.

What's your driver for trying to unify?

[adriaanm]

I don’t want to keep adding annotations for other axes. That’s an order of magnitude less scalable than the concern with the int for the severity. It could be an enum. Or a string. In any case, the meaning would be specific to the project and configurable.

…

On Mon, 11 Mar 2019 at 18:15, Dale Wijnand ***@***.***> wrote: That assumes those all naturally fit in an int axis, which I don't this is true. I think it's plausible to imagine some users wanting to see those poorPerformance warnings but others being fine with the apiMayChange ones, and others wanting the inverse. What's your driver for trying to unify? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7790 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFjyyEighIg7NtZ6WGMP54wGphwwQOsks5vVo9KgaJpZM4bOQAU> .

[dwijnand]

Would the enum be project-side too, or in the standard library? If it's project-side, then it would lose shared, universal semantic value (such as the apiMayChange + MiMa use-case). If it's library-side, then I'd be fine with it.

[som-snytt]

conditionalWarning.

[eed3si9n]

So if we reverse engineer the meaning of @restricted(...) that would be:

• advisory about the usage of the method
• sometimes about lifecycle of the method (incubating, deprecated, gone)
• that could manifest itself as compilation warnings or error

IIUC, the advisory category (apiMayChange, deprecation) would be label: String. If we want the tool to take special for the context, it does make sense to make label an enum like @dwijnand is saying. With String, we can envision irregularities such as "API May Change" vs "ApiMayChange" vs "APIMayChange"..

severity: Int should be able to denote forcefulness of the advisory, like Severity.AlwaysError, Severity.AlwaysWarn, Severity.Default etc.

• My @compileTimeError annotation becomes some category (ApiChange?) with Severity.AlwaysError.
• @deprecated annotation becomes Deprecation category with Severity.Default which gets promoted to a warning under -deprecation.

If we imagine -conditionalWarning:<advisory category> flag, -deprecation might desugar to -conditionalWarning:Deprecation.

[som-snytt]

I'll have to come up with a different name for the deprescalator. Or just make it a retronym for how depressed you are at the end of a compilation run.

[adriaanm]

The annotation and the mechanism to configure whether and how severely to warn should be in the stdlib & compiler, the rest should be configurable.

We could have a few default behaviours, like warning on using an internal member from another package.

[adriaanm]

I can't extemporaneously come up with a better name, but let's first focus on the semantics. I think we're getting somewhere!

[dwijnand]

To avoid the issues caused by adding variants to an enum / sealed hierarchy (around compile-time match exhaustion and runtime matching errors) maybe best have it be unsealed so consumers would have to handle the default case. Optionally with a (scala only) package private constructor, depending on if it should also be user-extendible or not.

• My @compileTimeError annotation becomes some category (ApiChange?)

Wouldn't it be in the Gone category?

And for that later 👨‍🎨 naming session:

• @deprecated annotation becomes Deprecation category with Severity.Default which gets promoted to a warning under -deprecation

Perhaps Allow, Pass, or Off (or, for pun points, Severity.Not 🙂)

Also, I think it would be good if the in-code names for the categories matched the command-line names. So if it's Deprecated in code then it should be Deprecated as a command-line argument. No needs-to-be-learnt string conversion functions #sbt-life-lessons

Finally, for the ultimate 👨‍🎨: @lint instead of @restricted?

[adriaanm]

I definitely want the category to be user-extensible/definable, which is why I suggest to let it be a String. What good is the type system if we really just want users to be able to come up with sensible names and provide them on the command line -- unless we port the type checker to bash, we won't get much benefit from static checks here.

The severity is likely a bit more closed world, so we could do an enum there.

I like @lint!

[dwijnand]

I definitely want the category to be user-extensible/definable, which is why I suggest to let it be a String. What good is the type system if we really just want users to be able to come up with sensible names and provide them on the command line -- unless we port the type checker to bash, we won't get much benefit from static checks here.

We'd be using the type system in the tools that consume those categories (like MiMa + ApiMayChange), which would provide a little more structure than "please, community, use this string here". So, perhaps:

class Category private[annotation]

object Category {
  final case object Deprecated extends Category
  final case object ApiMayChange extends Category
  final case class Custom(identifier: String) extends Category
}

[adriaanm]

I see little benefit to that. Some, but not enough to warrant the types. I think we can get most of the benefit by having a few predefined constants.

…

On Tue, 12 Mar 2019 at 10:39, Dale Wijnand ***@***.***> wrote: I definitely want the category to be user-extensible/definable, which is why I suggest to let it be a String. What good is the type system if we really just want users to be able to come up with sensible names and provide them on the command line -- unless we port the type checker to bash, we won't get much benefit from static checks here. We'd be using the type system in the tools that consume those categories (like MiMa + ApiMayChange), which would provide a little more structure than "please, community, use this string here". So, perhaps: class Category private[annotation] object Category { final case object Deprecated extends Category final case object ApiMayChange extends Category final case class Custom(identifier: String) extends Category { override def toString = s"Custom($identifier)" } } — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7790 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFjyw_Tmps1OPvLzfDygSjDYLP3tEUVks5vV3XvgaJpZM4bOQAU> .

[dwijnand]

String literals being so readily available I fear we'd immediately have both "deprecated" and "deprecation" as categories, because some users didn't see the Category.Deprecated constant.

[adriaanm]

I don't think that's a big deal. This is an optional, cultural thing that will mostly be driven from outside configuration outside the purview of the type checker. I would rather have zero friction to free choice of categories. It's not about correctness/exhaustivity but rather about being helpful. Having a few built-in types and then a Custom "cop-out" does not feel like a substantial UX gain to me.

If they want the benefits of the few built-in categories we'll eventually support, they'll have to read the docs and fix their spelling.

[dwijnand]

OK. And I guess we can attach documentation to those constants, detailing the intended semantics and usage of the category.

[eed3si9n]

I talked to @szeiger about the removal of the multi-param + and he said they are intentionally kept there with @deprecated for cross building purpose in 2.13, so I'll drop the commit.

@compileTimeError / @lint will be useful beyond that hopefully.

[adriaanm]

If we're going to get this annotation into 2.13.0, we need to agree on the design and merge this PR this week. I suggest delaying the multi-param infix errors until later -- we can introduce those in 2.13.x. The reporter configuration part can also come later.

For the annotation, I suggest:

final class restricted(message: String, label: String, defaultSeverity: String = "warning") extends scala.annotation.StaticAnnotation

In my mind, "lint" could be a label, but the more generic part is that we're restricting usage of a member (as a generalisation of deprecating). All arguments are stringly typed because we're going for flexibility and user-friendliness. There's little for the type checker to enfore here.

[adriaanm]

You could encode akka.annotation.ApiMayChange as @restricted("API is evolving", "api-evolving", "error"), and akka could build with something like -Wconfig: if (label == "api-evolving" && sourcePackage ~= "akka.*") severity = "ignore" (the config could be a custom subclass of Reporter that implements these rules in Scala).

[eed3si9n]

use case 1: library author really wants it to be an error

For this mode, we need something like @compileTimeError. As per the name, I'd be happy with @deprecatedError.

use case 2: library author provides some context, and library users decide the severity

This is the use case that generalizes @deprecated. That would look like @restricted minus defaultSeverity.

[eed3si9n]

@adriaanm Implemented @restricted + -Wconfig rules.

[adriaanm]

Wow, excellent! What do you think about also having @restricted subsume @deprecatedError?

[eed3si9n]

What do you think about also having @restricted subsume @deprecatedError?

My preference would be to keep @deprecatedError as a shorthand, assuming there's no facility in Scala to abstract an annotation with specific arguments.

[NthPortal]

I think a "mistake" label for methods whose use indicates a coding mistake would be nice

[SethTisue]

in our team meeting, @retronym mentioned JEP 277, which is very much in this space; it was implemented in JDK 9.

The Java designers considered encoding the reasons for the deprecation, but in the end, decided only to add a single forRemoval() method (and since(), which our annotation already has).

Among the relevant passages:

The exact reasons for deprecating an API are often too subtle to be expressed as flags or element values in the annotation. It is strongly recommended that the reasons for deprecating an API be described in that API's documentation comments. In addition, it is also recommended that potential replacement APIs be discussed and linked from the documentation

A string value has been proposed as a detail code. This appears to provide more flexibility, but it also introduces problems with weak typing and namespace conflicts, possibly leading to undetected errors

Previous versions of this proposal included a variety of "reason" codes including UNSPECIFIED, DANGEROUS, OBSOLETE, SUPERSEDED, UNIMPLEMENTED, and EXPERIMENTAL. These attempted to encode the reason for which an API was deprecated, the risks of using it, and also whether a replacement API is available. In practice, all of this information is too subjective be encoded as values in an annotation. Instead, this information should be described in the Javadoc documentation comment.

[adriaanm]

In the mentioned team meeting I fought pretty hard for this PR, but couldn't convince the others. So, closing, with regret. In summary, the conclusion was that we need to flesh out the design space more and probably submit a SIP.

[som-snytt]

Also forRemoval #7714

There is a certain wisdom in leveraging the work already done by the Java community.

However, the optional label in the other PR is for convenient suppression, not for explanatory purposes, so kinda orthogonal.