Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal to enhance the deprecated annotation #32726

Closed
bwilkerson opened this issue Mar 31, 2018 · 5 comments
Closed

Proposal to enhance the deprecated annotation #32726

bwilkerson opened this issue Mar 31, 2018 · 5 comments
Assignees
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). type-enhancement A request for a change that isn't a bug

Comments

@bwilkerson
Copy link
Member

Enhance the Deprecated annotation with additional information to support automated migration tooling.

The constructor for the class Deprecated should be enhanced to take named parameters whose values can be used by tools to automate the migration of use sites from the deprecated API to a newer API when one exists.

Some migrations will not be amenable to automated tooling and/or will be infrequent enough to not justify the time required to support them, but I believe that we can and should support the most common migrations.

One of the simplest, and probably the most common, use cases for this support is to rename a member. I propose that we begin with a single named parameter containing the new name. Below is sample text for the Dartdoc describing this parameter.

/**
 * ...
 *
 * If a [replacedBy] value is provided, then tooling can assume that the
 * annotated member has been replaced by some other member and that references
 * to the deprecated member should be updated to reference the replacing member.
 *
 * If the replacing member is in the same name scope as the deprecated member,
 * then the content of the [replacedBy] value is expected to contain the name of
 * the replacing member. For example, if a static field is being renamed, the
 * code might look something like the following:
 * ```
 * class C {
 *   @Deprecated(replacedBy: 'zero')
 *   static const int ZERO = 0;
 *
 *   static const int zero = 0;
 * }
 * ```
 * If the replacing member is a static member of a class and the deprecated
 * member is not defined in the same class, then the content of the [replacedBy]
 * value is expected to contain both the name of the class containing the
 * replacing member and the name of the replacing member, separated by a period.
 * For example, if a static field is being moved to a new class, the code might
 * look something like the following:
 * ```
 * class A {
 *   static const int origin = 0;
 * }
 *
 * class B {
 *   @Deprecated(replacedBy: 'A.origin')
 *   static const int zero = 0;
 * }
 * ```
 * If the [replacedBy] value contains anything other than what is described
 * above, then tooling can issue a diagnostic indicating that the contents
 * are invalid.
 */
const Deprecated(String expires, {String replacedBy});

The second most common use case is probably moving a member from one library to another. I believe that we can support this use case (in the future) by adding a second named parameter containing the URI of the library containing the replacing member.

/**
 * ...
 *
 * If a [fromLibrary] value is provided, then tooling can assume that the
 * replacing member is defined in a different library than the deprecated member
 * and that the library can be imported using the URI that is the content of the
 * [fromLibrary] value. If the URI is not an absolute URI then it will be
 * interpreted to be relative to the containing library.
 *
 * If a [fromLibrary] value is provided without a [replacedBy] value, then the
 * replacing member is assumed to have the same name as the deprecated member.
 */
const Deprecated(String expires, {String replacedBy, String fromLibrary});

Some additional use cases that we might consider supporting are changes to the number or types of function parameters or the number of type parameters. I believe that these could be supported by extending the interpretation of the replacedBy parameter's content, but have not done the design work related to them because I don't think we ought to try to support these cases at this time.

@bwilkerson bwilkerson added area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). type-enhancement A request for a change that isn't a bug labels Mar 31, 2018
@matanlurey
Copy link
Contributor

Honestly I'd rather see named constructors be used for this versus lots of optional arguments:

// Renamed/moved to another library.
const factory Deprecated.movedTo(String name, {String sourceLibrary});

// Being removed entirely without an explicit replacement.
const factory Deprecated.willBeRemoved();

@lrhn
Copy link
Member

lrhn commented Apr 2, 2018

An annotation like this is really a small language. The "replacedBy" string is interpreted by tools, in the static context of the declaration. It's like DartDoc references in that way, and restricted in the same ways. On top of that, it only allows you to specify renamings - a tool that applies the change can only change one reference to another, it can't change, say, a value to a function returning that value (like the change T defeaultValue to T orElse() requires).

I'm not sure it's really worth it as long as we are using strings as "change descriptors". We don't plan to make that many more renamings, and it can't handle complex changes anyway, so the value is limited.
Now, if we had an official syntax representation, and we could supply a function that rewrites such an AST into a modified AST as the argument, then it starts becoming generally useful.

(And yes, named constructors are probably better, as proven by all the examples in the proposal being invalid because they omit the positional "expires" argument. 😄)

@eernstg
Copy link
Member

eernstg commented Apr 4, 2018

I think we should aim for a very simple language here, but the main thing might well be the ability to name a transformation which is supported by some tool. That tool would then be able to traverse the source code, find each AST node where some deprecated feature is used, and use the name of the transformation to select and run a fix-up procedure on that node. It could do anything, including transforming the code to some extent and then putting up a dialog box saying "Please complete the following code such that ...[something]...".

@devoncarew
Copy link
Member

Here are some examples of recent deprecations (and, ~toolable deprecation transformations we'd like to support):

https://gist.github.com/zanderso/d554e6159f486e8f96759a4f9774a01a

@bwilkerson
Copy link
Member Author

I suspect that data-driven fixes satisfy most of the need for this proposed enhancement, so I'm closing this as not intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants