sdk
sdk copied to clipboard
dart-lang
New annotation request: `@IntentToAddClassModifier.kFinal` (and others)
Starting at Dart 3.x+, we have class modifiers, yay!
I'd like a way to mark an intent to adopt a modifier in a future release, so that:
- The analyzer can issue diagnostics that a to-be-added modifier will become breaking.
- The user can ignore said issue (it's not a release blocker) and attend to it as they prioritize.
As a straw-man, I'd propose adding IntentToAddClassModifier to package:meta:
// meta.dart
import 'meta_meta.dart';
/// Annotation marking a class as soon-to-add the equivalent class modifier.
///
/// Tools, such as the analyzer, can provide feedback if
/// - the annotation is associated with anything other than a class
/// - the annotation is associated with a class `C` and the corresponding class modifier would be a static error
@Target({TargetKind.classType})
enum IntentToAddClassModifier {
kAbstract,
kBase,
kFinal,
kInterface,
kSealed,
kMixin,
}
For example, I'd use it here: https://github.com/flutter/flutter/issues/142937.
/cc @pq @srawlins (I wrote this here versus dart-lang/linter because of the need to re-use analyzer logic for modifiers).
As a counter-strawman, how about:
/// Put this on a class to indicate that it should no longer be used in an
/// `extends` clause. Extending a class with this reports a deprecated lint
/// warning. Eventually, the class will be marked `final`, `interface`, or
/// `sealed`.
@DeprecatedExtends
/// Put this on a class or mixin to indicate that it should no longer be used
/// in an `implements` clause. Implementing a class or mixin with this reports
/// a deprecated lint warning. Eventually, the class or mixin will be marked
/// `final`, `base`, or `sealed`.
@DeprecatedImplements
/// Put this on a class or mixin to indicate that it should no longer be used
/// in a `with` clause. Mixing in a class or mixin with this reports a
/// deprecated lint warning. Eventually, the class or mixin will be marked
/// `final` or`sealed`.
@DeprecatedMixin
/// Put this on a class or mixin to indicate that it should no longer be
/// directly constructed (i.e. have one of its generative constructors invoked
/// outside of a subclass's initializer list). Invoking a non-factory
/// constructor on a class with this annotation reports a deprecated lint
/// warning. Eventually, the class will be marked `abstract` or `sealed`.
@DeprecatedConstruct
Maybe, I just like the idea of using the existing documentation and names used in the modifiers.
As a counter-strawman, how about:
I hope we don't have to repeat the same evaluation process as when we defined the modifiers.... But I'll add a proposal.
+1 for modeling the annotation API on the modifiers so authors only need to think once about how to express their intent. The transition from annotation to keyword is trivial do correctly.
-1 for using a k prefix on the names.
How about @willBeFinal, @willBeInterface, @willBeBase, @willBeSealed.
Yes please! Let's lean on user's existing understanding of the class modifier terms. That might not exist at the preferred/ideal levels yet, but that will slowly improve with time and tooling enhancements :)
Other than that, I like this idea!
Just to be clear k was to avoid having to understand how to make reserved words work, so I'm on board.
its my fault, I've got @matanlurey working down in the C++ mines so his brain is full of chromium style guide.
How about @willBeFinal, @willBeInterface, @willBeBase, @willBeSealed.
I like this idea.
As for the diagnostic specifically, I feel like we've talked about reporting something like this before (maybe in relation to the @since annotation?) but can't recall now. @bwilkerson?
I'll note that the comments on this thread are all about the form of the annotation, not the value of having such an annotation. I don't know whether that means that folks are generally in favor of the proposal, or whether it's just more fun to discuss the details. :-) I don't have a strong opinion yet, but I do have some questions.
This is very similar in some ways to deprecated in that it's warning of an upcoming breaking change. Are there other kinds of breaking changes that we should be considering at the same time?
How much value will this bring to users? How often are breaking changes like this being made? How many users are impacted by those changes? How much would advance notice help users migrate?
Are there better ways to help users migrate? Is there something we could do using dart fix or some other tooling that would be a better UX, and if so would it negate the value of these annotations?
If we were to add support like this, would we want to be able to use it in the dart: libraries to announce future breaking changes?
@lrhn
How much value will this bring to users? How often are breaking changes like this being made? How many users are impacted by those changes? How much would advance notice help users migrate?
Dart and Flutter both had a variety of breaking changes here for Dart 3 and annotations like these could have potentially helped users account for that earlier. A list of the SDK's changes this could have helped in Dart 3: https://dart.dev/resources/dart-3-migration#extends--implements (if the SDK could use package:meta :P).
I think in the short term, there's likely many packages created before class modifiers existed that haven't taken the step to tighten their class capabilities. This functionality could definitely help in that migration. However, after the ecosystem has been given a longer period to adapt, it does feel like the lint could slowly become less useful.
I see the longer term solution for new APIs is for developers to consider class capabilities earlier on (https://github.com/dart-lang/sdk/issues/54656). Sometimes we make the wrong decision though or project requirements change, so there will always be times we may need to retighten the class's capabilities and this issue's proposed functionality would still be useful.
If we were to add support like this, would we want to be able to use it in the dart: libraries to announce future breaking changes?
Consider the above, I do think that would be useful, same for the other annotations in package:meta =]
Are there other kinds of breaking changes that we should be considering at the same time?
I think another useful one that's been discussed a few times is @willBeRequired to indicate an optional named or positional parameter will be required in the future.
@bwilkerson: Your questions are valid, but I'd like to avoid a tit-for-tat where the onus is to answer every question posed.
Instead I'd suggest:
- this feature is as valuable as making breaking changes is valuable to a codebase
- we would make more breaking changes if the tooling to land the changes were better
- this annotation improves our ability to make breaking changes
... ergo, yes, it is valuable and we should do it (and also do it independently of analyzing where else we could help).
In terms of dart fix, it crossed my mind that perhaps this feature could be "emulated" by a clever fix, for example:
// Imagine a fix that did this:
import 'dart:ui';
- class MyPaintSubtype extends Paint {}
+ class MyPaintSubtype extends PaintDuringMigration {}
... and dart:ui could do something like:
final class Paint {}
@deprecated
class PaintDuringMigration extends Paint {}
I don't know if this is "better" than the annotation. It certainly is more work, and is likely more confusing for our users.
I think this is a reasonable request, probably useful, and probably something we'll use ourselves.
A deprecation is an early warning of a breaking change, preferably one that the user can prepare for. Something where, if you do the preparation, your code will keep working when the breaking change happens, because you are no longer depending on the existing behavior.
Until now most of the breaking changes that one can prepare for have been removals, but there are exceptions.
Not all removals can be prepared for. Something like removing a required parameter is not something one can prepare for, because not passing the parameter is currently an error. The only workaround is to not use the function at all, but it's not the entire function that goes away. Marking the parameter @deprecated doesn't help.
(But if the parameter is made optional when it's deprecated, the it would work.)
Not all things that can be prepared for are removals. Making an optional parameter required can be prepared for by starting to pass a value. We have no marker for that. It's probably also fairly rare (I haven't noticed any need for it).
Class modifiers changes that, big time. First of most of them are largely breaking changes when added to existing classes, and no existing classes had the modifiers, so there has been (and probably still is) a large initial burst of breaking changes. It's also something one will likely want to be adding to APIs in the future, as they evolve, so the need keeps being there.
We made a lot of classes in the SDK libraries final. We could do that only because we cheat, and allow every pre-3.0 library to ignore those modifiers. (Yes, you can extend a final class from the platform libraries, all you need to do is add //@dart=2.19. Please don't.)
If we had had deprecation-annotations like these, we would have added them to the 2.19 release (well, we would have liked to add them there, in reality we were doing the migration far to late for that). Then every pre-3.0 library would see the warning of "this thing will stop being extensible, stop extending it" and would be encouraged to adapt. If we had that, we might even consider removing the exception early, say in Dart 3.6 or so, because all pre-3.0 code would have gotten warnings for months at that time.
So that would be nice.
I think such deprecations annotations should be phrased in terms of what goes away. The ability to implement, the ability to extend. Maybe a sealed type will stop being sealed, which will take away exhaustibility. Maybe an enum will stop being an enum (which would also make it no longer implement Enum), because it can't guarantee not breaking exhaustibility in the future.
If we do put the annotations into the SDK libraries, I don't want a big API surface. I'd suggest:
@Deprecated.implement() // Stop implementing.
@Deprecated.extend() // Stop extending.
@Deprecated.mixin() // Stop mixing in.
@Deprecated.construct() // Stop instantiating, the class will become abstract.
@Deprecated.exhaust() // Stop assuming switches are exhaustive, add a default case. Applies to sealed classes and `enum`s.
// Maybe, but speculative, I've never needed them.
@Deprecated.optional() // For parameters that stop being optional. Start passing that argument.
@Deprecated.generic() // For declarations the will lose their type parameters. Switch to raw types, now!
which are all constructors on Deprecated, which can take whatever arguments we want them to (but probably a "how to migrate" message).
We can still use the plain @Deprecated() for things that are simply going away ("Stop using this, at all").
These are all breakage that one can respond to. Not necessarily by doing the same thing in another way, maybe there is no longer a way to do what you were doing before, but at least you'll now know that it will stop working.
I'm in!
I really like @lrhn's proposal. A crack at an implementation is up for consideration here: https://dart-review.googlesource.com/c/sdk/+/352940. Any feedback appreciated.
@Deprecated.extend() could mean the class will become final or interface, or could remove all generative constructors. We should ignore he deprecation within the same library, even if there is a small chance it was intended to apply for the latter reason.
@natebosch added a few questions to the PR that deserve broader attention:
Will these all have separate diagnostic names, and so separate ways to ignore them?
My thinking here is, yes.
Do we think we're going to need "from same package" variants?
I'm not sure. I'd prefer not but haven't thought it through completely. Feedback welcome!
Will these all have separate diagnostic names, and so separate ways to ignore them?
My thinking here is, yes.
I agree. I believe that users will want to be able to ignore one kind of deprecation while not ignoring every kind of deprecation.
Do we think we're going to need "from same package" variants?
The existing "from same package" diagnostic is separate because it's a lint rather than a warning. Some users found that it was too noisy to be told about deprecation issues within their own code. As a result we converted it from a warning to a lint. I don't see any reason to suggest that the same principle wouldn't apply in this situation, so yes, we should have the distinction and the "from same package" variants should be lints.
Thanks Brian!
Some users found that it was too noisy to be told about deprecation issues within their own code.
I wonder if these kinds of deprecations will be as noisy? If not, I wonder if ignores wouldn't be sufficient for the common case.
variants should be lints.
That said, if we do need these variants, I agree they should be lints.
I'm curious, @matanlurey: as someone thinking about the code evolution motivating this issue, what do you think?
From a user perspective, the alternative is "I have to read the CHANGELOG manually", so any sort of warning is better than none. From a developer perspective, I don't really care if there is separate diagnostics issued - again, my alternative before was "no diagnostics".
I think we're making this too hard on ourselves. Personally, I would just have these emit deprecated_class_capability and deprecated_class_capability_same_package, and not worry about having 10+ different messages.
From a user perspective, the alternative is "I have to read the CHANGELOG manually", so any sort of warning is better than none.
Based on what you wrote, I'm not sure the original question was clear. The question is, as the author of a package, if you add one of these annotations to a class, do you want to be told about all the places in your own code that will need to be updated?
Some users say "yes, that makes it easier for me to find it". Others say "no, I don't want to change the rest of the code until I've actually added the modifier to the class, so this is noise in the meantime".
I think we're making this too hard on ourselves. Personally, I would just have these emit
deprecated_class_capabilityanddeprecated_class_capability_same_package, and not worry about having 10+ different messages.
From a tooling perspective, having multiple forms allows us to have messages, documentation, and maybe even fixes that are specific to the kind of deprecation being reported. If all we know is that some capability is being removed, but don't know which capability, there's not much that we can do to help the user.
It seems to me that from a user's perspective, if we have a single diagnostic then the user needs to hope that the API author included more information in the message. Otherwise they have no idea what needs to change and the diagnostic becomes non-actionable.
By doing the extra work to have multiple diagnostics I think we make it easier for our users, both API authors and API consumers.
🤷🏼 I was asked for my opinion and I gave it - I do think sometimes we spend too much time on a hypothetical perfect - but you/your team own the experience and I trust you to do whatever makes sense.
To be clear, I really just wanted to make sure that we had your opinion about the right question.
I guess to be clear: // ignore seems fine for the common case (the alternative/status quo is much worse) and I don't mind if variants are lints/hints/etc.
@bwilkerson:
To be clear, I really just wanted to make sure that we had your opinion about the right question.
Just to follow up - I re-read my comment and I think I got unnecessarily jerky here, sorry about that. I do appreciate you're looking at this - I am not sure exactly what the right UX is, only that I support what makes it easier for the analyzer team to own and roll out.
I would prefer that all these "can't implement", "can't extend", etc. warnings do not show up for code in the same library. For two reasons.
- Code in the same library likely won't break at all. If a class becomes
final, another class in the same library can still implement or extend it. - If something breaks when the deprecated behavior is removed, the library author will notice. Immediately. It's not like they need advance warning, because they won't be able to publish the breaking change without fixing their code first.
That's only for the same library. Having exceptions for other code in the same package is different. I can see having optional lints for that, the same "Don't warn about my own code, I'm well aware of it and don't need to change it until I do the actual rewrite" as for normal deprecation.
(Making a single constructor non-generative is tricky. Maybe we should allow annotating a single generative constructor with @Deprecated.extend(), meaning that it will become a factory constructor, and can no longer be used for extending the class. You can still use it for everything else, it really is only the extending use-case that gets broken. There might be other generative constructors left, so it doesn't make the class non-extensible.)