[L] Change in dart/sdk[main]: Analyzer warnings: add support for @Deprecation.subclass
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted and added 4 comments![]( "Open in Gerrit")
Votes added by Konstantin Shcheglov
| Code-Review | +1 |
4 comments
class B implements A {}Do we care about `with` when older language version?
```
// @dart = 2.12
class A {}
class B extends Object with A {}
```
if (classElement == null) return;FWIW, at this point it must have a fragment and element.
if (element is ClassElement &&It looks that we repeat conditions. Maybe update two previous checks to go from AST to elements, and then check elements as here?
class Bar implements Foo {}Do we want similar tests for mixins (`implements` and `on`) and enums?
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 4 comments![]( "Open in Gerrit")
class B implements A {}Do we care about `with` when older language version?
```
// @dart = 2.12
class A {}class B extends Object with A {}
```
Good question; it's not mentioned in https://github.com/dart-lang/sdk/issues/60504, but a good question for @l...@google.com
if (classElement == null) return;FWIW, at this point it must have a fragment and element.
Do you mean it's desirable to use `declaredFragment!.element`? Is there a shorthand for this, or maybe `declaredFragmentOrThrow` like `typeOrThrow`?
if (element is ClassElement &&It looks that we repeat conditions. Maybe update two previous checks to go from AST to elements, and then check elements as here?
I think it would be some hoops to jump through. We'd do a ClassDeclaration case, and a MixinDeclaration case, to set an `Element` variable. Then we'd have to again do a `ClassElement` case to access `.isImplementableOutside` and a MixinElement case to access `.isImplementableOutside`. *shrug*
class Bar implements Foo {}Do we want similar tests for mixins (`implements` and `on`) and enums?
Hmm, with `on`, do you mean how testing how this annotation affects super-constraints? It isn't specified to affect that, but we could. I need to look at the rules of `sealed` and `final` again.
And enums cannot be implemented. Do you mean an enum that implements a class?
Related details
- Brian Wilkerson
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 6 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
6 comments
status: needsFixThere is an existing `RemoveAnnotation` fix that might be usable here.
var isSubclass = value.getField('_isSubclass')?.toBoolValue() ?? false;I wonder whether it would be worthwhile to have extension methods for these tests. I seem to remember seeing this same pattern in a previous CL, and we might need to change all of them to look for specific enum values. Having one place to update might be good.
just remove the relevant `extends` clause or remove the class name fromWhile it seems kind of trivial, consider adding a code snippet that shows the original code after the extends clause has been removed. (See below for rationale.)
Also, to be more consistent with other docs, consider something like
If the annotation contains a description of how to deal with not being able to subclass the class, then try following those directions.
If the annotation doesn't contain a description, or if the described approach isn't appropriate for your case, then remove the relevant `extends` clause or remove the class name from the `implements` clause:
```dart
import 'package:p/p.dart';
class D {}
```
The analyzer produces this diagnostic when anything other than annit: "an" --> "a"
[`Deprecated.subclass`][meta-deprecated-subclass]. A subclassableI think this link can't work because the constructor doesn't yet exist. Given that the docs can be published at any time, we should probably remove the link for now and then try to remember to put them back in after the constructors have been defined.
Remove the annotation.As silly as it seems, we should add code to which the recommendation has been applied:
Remove the annotation:
```dart
sealed class C {}
```
It's more consistent, and the example might help an LLM do a better job of fixing the problem.
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Konstantin Shcheglov
| Code-Review | +1 |
3 comments
if (classElement == null) return;Samuel RawlinsFWIW, at this point it must have a fragment and element.
Do you mean it's desirable to use `declaredFragment!.element`? Is there a shorthand for this, or maybe `declaredFragmentOrThrow` like `typeOrThrow`?
No, we don't, I was just adding null asserts directly.
Should we add one? Maybe.
I'm considering a different name, like `declaredFragmentMust`.
There is no "or" in the places where we invoke it :-)
if (element is ClassElement &&Samuel RawlinsIt looks that we repeat conditions. Maybe update two previous checks to go from AST to elements, and then check elements as here?
I think it would be some hoops to jump through. We'd do a ClassDeclaration case, and a MixinDeclaration case, to set an `Element` variable. Then we'd have to again do a `ClassElement` case to access `.isImplementableOutside` and a MixinElement case to access `.isImplementableOutside`. *shrug*
But this what we do here: check for `is ClassElement` and then its permissions, right?
I'm totally can be wrong.
class Bar implements Foo {}Samuel RawlinsDo we want similar tests for mixins (`implements` and `on`) and enums?
Hmm, with `on`, do you mean how testing how this annotation affects super-constraints? It isn't specified to affect that, but we could. I need to look at the rules of `sealed` and `final` again.
And enums cannot be implemented. Do you mean an enum that implements a class?
Yes, enum that implements a class.
And yes, `on` super-constraints. Theoretically these are almost equivalent to mixin implementing these (with ability for `super`).
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 8 comments![]( "Open in Gerrit")
Samuel RawlinsDo we care about `with` when older language version?
```
// @dart = 2.12
class A {}class B extends Object with A {}
```
Good question; it's not mentioned in https://github.com/dart-lang/sdk/issues/60504, but a good question for @l...@google.com
I've implemented it for now. It makes sense to me. If we want to change that later, it is non-breaking to stop reporting it.
if (classElement == null) return;Samuel RawlinsFWIW, at this point it must have a fragment and element.
Konstantin ShcheglovDo you mean it's desirable to use `declaredFragment!.element`? Is there a shorthand for this, or maybe `declaredFragmentOrThrow` like `typeOrThrow`?
No, we don't, I was just adding null asserts directly.
Should we add one? Maybe.I'm considering a different name, like `declaredFragmentMust`.
There is no "or" in the places where we invoke it :-)
Done
if (element is ClassElement &&Samuel RawlinsIt looks that we repeat conditions. Maybe update two previous checks to go from AST to elements, and then check elements as here?
Konstantin ShcheglovI think it would be some hoops to jump through. We'd do a ClassDeclaration case, and a MixinDeclaration case, to set an `Element` variable. Then we'd have to again do a `ClassElement` case to access `.isImplementableOutside` and a MixinElement case to access `.isImplementableOutside`. *shrug*
But this what we do here: check for `is ClassElement` and then its permissions, right?
I'm totally can be wrong.
OK I gave it a try and it is shorter, more concise. I like it, thanks!
just remove the relevant `extends` clause or remove the class name fromWhile it seems kind of trivial, consider adding a code snippet that shows the original code after the extends clause has been removed. (See below for rationale.)
Also, to be more consistent with other docs, consider something like
If the annotation contains a description of how to deal with not being able to subclass the class, then try following those directions.
If the annotation doesn't contain a description, or if the described approach isn't appropriate for your case, then remove the relevant `extends` clause or remove the class name from the `implements` clause:
```dart
import 'package:p/p.dart';class D {}
```
Done
The analyzer produces this diagnostic when anything other than anSamuel Rawlinsnit: "an" --> "a"
Done
[`Deprecated.subclass`][meta-deprecated-subclass]. A subclassableI think this link can't work because the constructor doesn't yet exist. Given that the docs can be published at any time, we should probably remove the link for now and then try to remember to put them back in after the constructors have been defined.
Done
Remove the annotation.As silly as it seems, we should add code to which the recommendation has been applied:
Remove the annotation:
```dart
sealed class C {}
```It's more consistent, and the example might help an LLM do a better job of fixing the problem.
Done
class Bar implements Foo {}Samuel RawlinsDo we want similar tests for mixins (`implements` and `on`) and enums?
Konstantin ShcheglovHmm, with `on`, do you mean how testing how this annotation affects super-constraints? It isn't specified to affect that, but we could. I need to look at the rules of `sealed` and `final` again.
And enums cannot be implemented. Do you mean an enum that implements a class?
Yes, enum that implements a class.
And yes, `on` super-constraints. Theoretically these are almost equivalent to mixin implementing these (with ability for `super`).
OK, I see. This makes sense to me. Added impl and tests.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted Code-Review+1![]( "Open in Gerrit")
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: pkg/analyzer/messages.yaml
Insertions: 8, Deletions: 2.
@@ -24050,6 +24050,10 @@
Follow any directions found in the `Deprecation.extend` annotation, or
just remove the `extends` clause.
+
+ ```dart
+ class D {}
+ ```
DEPRECATED_EXTENDS_FUNCTION:
sharedName: DEPRECATED_SUBTYPE_OF_FUNCTION
problemMessage: "Extending 'Function' is deprecated."
@@ -24121,6 +24125,10 @@
Follow any directions found in the `Deprecation.implement` annotation, or
just remove the class name from the `implements` clause.
+
+ ```dart
+ class D {}
+ ```
DEPRECATED_IMPLEMENTS_FUNCTION:
sharedName: DEPRECATED_SUBTYPE_OF_FUNCTION
problemMessage: "Implementing 'Function' has no effect."
@@ -24234,8 +24242,6 @@
`extends` clause or remove the class name from the `implements` clause:
```dart
- import 'package:p/p.dart';
-
class D {}
```
DOC_DIRECTIVE_HAS_EXTRA_ARGUMENTS:
```
Change information
Analyzer warnings: add support for @Deprecation.subclass
Work towards https://github.com/dart-lang/sdk/issues/60504
* The annotation causes certain usage to generate a warning (+ tests).
* Possible fixes for this new warning are offered (+ tests).
* Placing the annotation on an invalid element generates a different
warning (+ tests).
- M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
- M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
- M pkg/analysis_server/test/src/services/correction/fix/remove_name_from_declaration_clause_test.dart
- M pkg/analyzer/lib/src/diagnostic/diagnostic_code_values.g.dart
- M pkg/analyzer/lib/src/error/annotation_verifier.dart
- M pkg/analyzer/lib/src/error/best_practices_verifier.dart
- M pkg/analyzer/lib/src/error/codes.g.dart
- M pkg/analyzer/lib/src/error/deprecated_functionality_verifier.dart
- M pkg/analyzer/lib/src/test_utilities/mock_sdk.dart
- M pkg/analyzer/messages.yaml
- M pkg/analyzer/test/src/diagnostics/deprecated_implement_test.dart
- A pkg/analyzer/test/src/diagnostics/deprecated_subclass_test.dart
- A pkg/analyzer/test/src/diagnostics/invalid_deprecated_subclass_annotation_test.dart
- M pkg/analyzer/test/src/diagnostics/test_all.dart
Code-Review: +1 by Konstantin Shcheglov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |