From 19f129e9469c7db3e171e18acbda8dbc19a86d69 Mon Sep 17 00:00:00 2001 From: Brian Wilkerson Date: Tue, 14 Apr 2026 12:08:08 -0700 Subject: [PATCH] Update diagnostic docs so that more can be validated This uses the previously added `%ignore=` syntax to enable validation of the diagnostic documentation for several diagnostics that were previously not being validated. In the process I also cleaned up a few of the code snippets used in those docs in order to minimize the number of ignored diagnostics. I removed the newly validated diagnostics from the list in the validator, and improved the comments for the ones that are still being ignored to show why they're ignored and point to some possible future remediations that would allow them to also be validated. Change-Id: I46b0eeaab86740ba82784987e84f3e93046d39f0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/494941 Reviewed-by: Paul Berry Commit-Queue: Brian Wilkerson --- pkg/_fe_analyzer_shared/messages.yaml | 1 + pkg/analyzer/messages.yaml | 40 +- .../test/verify_diagnostics_test.dart | 349 ++++++++++-------- pkg/linter/messages.yaml | 31 +- 4 files changed, 237 insertions(+), 184 deletions(-) diff --git a/pkg/_fe_analyzer_shared/messages.yaml b/pkg/_fe_analyzer_shared/messages.yaml index 4ef4472e768..0fe66d4a520 100644 --- a/pkg/_fe_analyzer_shared/messages.yaml +++ b/pkg/_fe_analyzer_shared/messages.yaml @@ -2620,6 +2620,7 @@ primaryConstructorBodyWithoutDeclaration: a primary constructor body but doesn't declare a primary constructor: ```dart + %ignore=undefined_identifier class C { [!this!] : assert(x > 0); } diff --git a/pkg/analyzer/messages.yaml b/pkg/analyzer/messages.yaml index 0ff00eb7053..fe6a7e1cfe1 100644 --- a/pkg/analyzer/messages.yaml +++ b/pkg/analyzer/messages.yaml @@ -1164,7 +1164,7 @@ CompileTimeErrorCode: import 'a.dart'; import 'b.dart'; - void f([!C!] c1, [!C!] c2) {} + void f(A a, [!C!] c1, [!C!] c2) {} ``` #### Common fixes @@ -1175,7 +1175,7 @@ CompileTimeErrorCode: ```dart import 'a.dart'; - void f(C c1, C c2) {} + void f(A a, C c1, C c2) {} ``` If the name is still defined by more than one library, then add a `hide` @@ -1185,7 +1185,7 @@ CompileTimeErrorCode: import 'a.dart' hide C; import 'b.dart'; - void f(C c1, C c2) {} + void f(A a, C c1, C c2) {} ``` If you must be able to reference more than one of these types, then add a @@ -1196,7 +1196,7 @@ CompileTimeErrorCode: import 'a.dart' as a; import 'b.dart' as b; - void f(a.C c1, b.C c2) {} + void f(a.A a, a.C c1, b.C c2) {} ``` ambiguousSetOrMapLiteralBoth: type: compileTimeError @@ -3564,7 +3564,7 @@ CompileTimeErrorCode: ```dart import 'dart:convert' deferred as convert; - const json2 = [!convert.JsonCodec()!]; + const json2 = [!convert.JsonCodec!](); ``` #### Common fixes @@ -5828,7 +5828,8 @@ CompileTimeErrorCode: internal library: ```dart - export [!'dart:_interceptors'!]; + %ignore=uri_does_not_exist + [!export 'dart:_interceptors';!] ``` #### Common fixes @@ -6094,9 +6095,8 @@ CompileTimeErrorCode: field: ```dart - %language=2.9 extension E on String { - String [!s!]; + String [!s!] = ''; } ``` @@ -6108,8 +6108,6 @@ CompileTimeErrorCode: ```dart extension E on String { String get s => ''; - - void s(String value) => print(s); } ``` @@ -6126,7 +6124,7 @@ CompileTimeErrorCode: using a getter and setter pair backed by a static `Expando`: ```dart - extension E on SomeType { + extension E on String { static final _s = Expando(); String get s => _s[this] ?? ''; @@ -7844,6 +7842,7 @@ CompileTimeErrorCode: `num` isn't a subtype of `int`: ```dart + // @dart = 3.8 class C { num get [!x!] => 0; @@ -8412,6 +8411,7 @@ CompileTimeErrorCode: internal library: ```dart + %ignore=uri_does_not_exist import [!'dart:_interceptors'!]; ``` @@ -13343,6 +13343,7 @@ CompileTimeErrorCode: constructor is marked with `async`: ```dart + %ignore=invalid_modifier_on_constructor class C { factory C() [!async!] { return C._(); @@ -14501,6 +14502,7 @@ CompileTimeErrorCode: directive with an import directive: ```dart + %ignore=unused_import import 'a.dart'; ``` partOfUnnamedLibrary: @@ -14982,9 +14984,10 @@ CompileTimeErrorCode: import prefix and the name of a function: ```dart - import 'dart:math' as f; + %ignore=undefined_method,unused_import + import 'dart:math' as [!f!]; - int [!f!]() => f.min(0, 1); + int f() => f.min(0, 1); ``` #### Common fixes @@ -16012,6 +16015,7 @@ CompileTimeErrorCode: ```dart %language=2.9 + %ignore=not_assigned_potentially_non_nullable_local_variable void f() { print([!i!]); int i = 5; @@ -18794,6 +18798,7 @@ CompileTimeErrorCode: `async` keyword: ```dart + %ignore=unused_local_variable void f(p) { [!await!] p; } ``` @@ -19872,8 +19877,9 @@ CompileTimeErrorCode: modifier: ```dart + %ignore=illegal_async_return_type Stream get digits async { - yield* [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; + [!yield* [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];!] } ``` @@ -23390,6 +23396,7 @@ ParserErrorCode: `_x` starts with an underscore: ```dart + // @dart=3.11 class C { void m({int [!_x!] = 0}) {} } @@ -24575,6 +24582,7 @@ StaticWarningCode: The following code produces this diagnostic because `x` can't be `null`: ```dart + %ignore=dead_code int f(int x) { return x ?? [!0!]; } @@ -24583,6 +24591,7 @@ StaticWarningCode: The following code produces this diagnostic because `f` can't be `null`: ```dart + %ignore=dead_code class C { int f = -1; @@ -28533,6 +28542,7 @@ WarningCode: function `g` isn't a constant: ```dart + %ignore=experimental_member_use import 'package:meta/meta.dart' show mustBeConst; int f(int value) => g([!value!]); @@ -28546,6 +28556,7 @@ WarningCode: with a constant: ```dart + %ignore=experimental_member_use import 'package:meta/meta.dart' show mustBeConst; const v = 3; @@ -30332,6 +30343,7 @@ WarningCode: `null`, so the comparison always evaluates to `false`: ```dart + %ignore=dead_code void f(int x) { if (x [!== null!]) { throw ArgumentError("x can't be null"); diff --git a/pkg/analyzer/test/verify_diagnostics_test.dart b/pkg/analyzer/test/verify_diagnostics_test.dart index 4614c605547..07d2e439ea1 100644 --- a/pkg/analyzer/test/verify_diagnostics_test.dart +++ b/pkg/analyzer/test/verify_diagnostics_test.dart @@ -33,152 +33,12 @@ class DocumentationValidator { /// ony include docs that cannot be verified because of missing support in the /// verifier. static const List unverifiedDocs = [ - // - // The following can't currently be verified because the examples aren't - // Dart code. - // - 'included_file_parse_error', - 'parse_error', + // The following diagnostics can't be verified because the examples aren't + // Dart code. The verifier needs to add the ability to verify YAML snippets + // and to use a YAML snippet as the example. If we can do this based on the + // class of the diagnostic, then there will be less chance of a false + // positive. 'analysis_option_deprecated', - 'deprecated_lint', - 'duplicate_rule', - 'included_file_warning', - 'include_file_not_found', - 'incompatible_lint', - 'invalid_option', - 'invalid_section_format', - 'plugins_in_inner_options', - 'recursive_include_file', - 'removed_lint', - 'undefined_lint', - 'unrecognized_error_code', - 'unsupported_option_with_legal_value', - 'unsupported_value', - - // Needs to be able to specify two expected diagnostics. - 'ambiguous_import', - // TODO(kallentu): This is always reported with - // `argument_type_not_assignable` or is reported as - // `const_eval_throws_exception` in const constructor evaluation. - 'const_constructor_param_type_mismatch', - // Produces two diagnostics when it should only produce one. - 'const_deferred_class', - // The mock SDK doesn't define any internal libraries. - 'export_internal_library', - // Also reports subtype_of_base_or_final_is_not_base_final_or_sealed - 'extends_disallowed_class', - // The following codes produce two diagnostics because they illustrate a - // cycle. - 'extension_type_implements_itself', - 'extension_type_representation_depends_on_itself', - // Not reported with `getter-setter-error` feature enabled. - 'getter_not_subtype_setter_types', - // Has code in the example section that needs to be skipped (because it's - // part of the explanatory text not part of the example), but there's - // currently no way to do that. - 'invalid_implementation_override', - // Produces two diagnostics when it should only produce one. We could get - // rid of the invalid error by adding a declaration of a top-level variable - // (such as `JSBool b;`), but that would complicate the example. - 'import_internal_library', - // Produces two diagnostics when it should only produce one. - 'invalid_uri', - // No example, by design. - 'missing_dart_library', - // Produces two diagnostics when it should only produce one. - 'non_sync_factory', - // Need a way to make auxiliary files that (a) are not included in the - // generated docs or (b) can be made persistent for fixes. - 'part_of_non_part', - // Produces multiple diagnostics when it should only produce one. - 'prefix_collides_with_top_level_member', - // Produces two diagnostics for clarity. - 'primary_constructor_body_without_declaration', - // Produces two diagnostic out of necessity. - 'recursive_compile_time_constant', - // Produces two diagnostic out of necessity. - 'recursive_constructor_redirect', - // Produces two diagnostic out of necessity. - 'recursive_interface_inheritance', - // Produces two diagnostics out of necessity. - 'referenced_before_declaration', - // Produces two diagnostic out of necessity. - 'top_level_cycle', - // Produces two diagnostic out of necessity. - 'type_alias_cannot_reference_itself', - // Produces two diagnostic out of necessity. - 'type_parameter_supertype_of_its_bound', - // Produces the diagnostic unused_local_variable when it shouldn't. - 'undefined_identifier_await', - // Produces multiple diagnostic because of poor recovery. - 'yield_each_in_non_generator', - - // This is not reported after 2.12, and the examples don't compile after 3.0. - 'field_initializer_in_struct', - // This is not reported after 2.12, and the examples don't compile after 3.0. - 'field_in_struct_with_initializer', - - // This no longer works in 3.0. - 'deprecated_colon_for_default_value', - // The code has been replaced but is not yet removed. - 'deprecated_member_use', - - // Need a way to specify the existance of files whose content is irrelevant. - 'always_use_package_imports', - // Missing support for example files outside of `lib`. - 'avoid_relative_lib_imports', - // The example isn't being recognized as a flutter app. We might need to - // build a pubspec.yaml when analyzing flutter code. - 'avoid_web_libraries_in_flutter', - // Produces a body_might_complete_normally. - 'control_flow_in_finally', - // Missing support for creating an indirect dependency on a package. - 'depend_on_referenced_packages', - // Missing support for specifying the name of the test file. - 'file_names', - // Produces an unused import diagnostic. - 'implementation_imports', - // Doesn't produce a lint for the second example, even though the analyzer - // does when the example is pasted into a file. - 'prefer_inlined_adds_single', - // No mock 'test' package, no good library annotations in 'meta'. - 'library_annotations', - // Produces an unused import diagnostic. - 'library_prefixes', - // Produces an unused element diagnostic. - 'library_private_types_in_public_api', - // Missing support for YAML files. - 'package_names', - // The lint does nothing. - 'package_prefixed_library_names', - // Need a way to specify the existance of files whose content is irrelevant. - 'prefer_relative_imports', - // The test file is in a basic workspace, so it can't have public API. I - // think we'd need to add a `pubspec.yaml` file to the example. - 'public_member_api_docs', - // Missing support for YAML files. - 'secure_pubspec_urls', - // The test framework doesn't yet support lints in non-dart files. - 'sort_pub_dependencies', - // Doesn't produce a lint for the first example, even though the analyzer - // does when the example is pasted into a file. - 'unnecessary_lambdas', - // Produces an unused_field warning. - 'use_setters_to_change_properties', - // Extra warning. - 'recursive_getters', - - // Has `language=2.9` - 'extension_declares_instance_field', - - // Produces the newer private_named_non_field_parameter diagnostic instead - // as part of the "private named parameters" feature. - 'private_optional_parameter', - - // - // The following can't currently be verified because the examples aren't - // Dart code. - // 'asset_does_not_exist', 'asset_directory_does_not_exist', 'asset_field_not_list', @@ -188,39 +48,204 @@ class DocumentationValidator { 'asset_path_not_string', 'dependencies_field_not_map', 'deprecated_field', + 'deprecated_lint', + 'duplicate_rule', 'flutter_field_not_map', + 'included_file_parse_error', + 'included_file_warning', + 'include_file_not_found', + 'incompatible_lint', 'invalid_dependency', + 'invalid_option', 'invalid_platforms_field', + 'invalid_section_format', 'missing_name', 'missing_dependency', 'name_not_string', + 'package_names', 'path_does_not_exist', 'path_not_posix', 'path_pubspec_does_not_exist', + 'parse_error', 'platform_value_disallowed', + 'plugins_in_inner_options', + 'recursive_include_file', + 'removed_lint', + 'removed_lint_use', + 'secure_pubspec_urls', + 'sort_pub_dependencies', 'unknown_platform', + 'undefined_lint', 'unnecessary_dev_dependency', + 'unrecognized_error_code', + 'unsupported_option_with_legal_value', + 'unsupported_value', 'workspace_field_not_list', 'workspace_value_not_string', 'workspace_value_not_subdirectory', - // Produces two diagnostics out of necessity. - 'dead_null_aware_expression', - - // Reports final_class_extended_outside_of_library - 'deprecated_extends_function', - // Doesn't apply to Dart files. - // TODO(brianwilkerson): Provide better support for non-Dart files. - 'removed_lint_use', - // Produces more than one error range by design. - // TODO(srawlins): update verification to allow for multiple highlight ranges. + // The following diagnostics can't be verified because they necessarily + // produce more than one diagnostic. This is typically because of a conflict + // between two or more declarations, neither of which is obviously the + // better declaration to flag, and we have chosen to create a diagnostic for + // all of them. + 'ambiguous_import', + 'extension_type_implements_itself', + 'extension_type_representation_depends_on_itself', + 'recursive_compile_time_constant', + 'recursive_constructor_redirect', + 'recursive_interface_inheritance', 'text_direction_code_point_in_comment', - // Produces more than one error range by design. 'text_direction_code_point_in_literal', - // Produces two diagnostics out of necessity. - 'unnecessary_null_comparison_never_null_false', - // Produced two diagnostics because `mustBeConst` is experimental. - 'non_const_argument_for_const_parameter', + 'top_level_cycle', + 'type_alias_cannot_reference_itself', + 'type_parameter_supertype_of_its_bound', + + // const_constructor_param_type_mismatch (analyzer) + // - Expected an error with code const_constructor_param_type_mismatch, + // found const_eval_throws_exception (example 0). + // + // Based on the TODO comment below, it appears that this diagnostic is never + // reported, and it should be marked as removed. + // + // TODO(kallentu): This is always reported with + // `argument_type_not_assignable` or is reported as + // `const_eval_throws_exception` in const constructor evaluation. + 'const_constructor_param_type_mismatch', + + // invalid_implementation_override (analyzer) + // - No error range in example + // - Expected no errors but found 1 (example 1): + // undefined_class (7, 1) Undefined class 'B'. + // + // Has code in the example section that needs to be skipped (because it's + // part of the explanatory text not part of the example), but there's + // currently no way to do that. We could try to rewrite the text so that all + // of the code is in a single snippet, or we could introduce a way to skip + // some code blocks. + 'invalid_implementation_override', + + // invalid_uri (analyzer) + // - Expected an error with code invalid_uri, found uri_does_not_exist + // (example 0). + // + // It's possible that this diagnostic is no longer reported. + 'invalid_uri', + + // yield_each_in_non_generator (analyzer) + // - No error range in example + // - Expected no errors but found 2 (example 0): + // - undefined_identifier (29, 5) Undefined name 'yield'. + // - body_might_complete_normally (18, 6) The body might complete + // normally, causing 'null' to be returned, but the return type, + // 'Iterable', is a potentially non-nullable type. + 'yield_each_in_non_generator', + + // deprecated_colon_for_default_value (analyzer) + // - Expected an error with code deprecated_colon_for_default_value, found + // obsolete_colon_for_default_value (example 0). + // + // This no longer works in 3.0 and should be marked as removed. + 'deprecated_colon_for_default_value', + + // deprecated_member_use (analyzer) + // - Expected an error with code deprecated_member_use, found + // undefined_class (example 0). + // + // The example needs to have a definition of `C` that is marked as + // deprecated. + 'deprecated_member_use', + + // avoid_relative_lib_imports (linter) + // - Expected one error but found 2 (example 0): + // - uri_does_not_exist (7, 15) Target of URI doesn't exist: '../lib/a.dart'. + // - avoid_relative_lib_imports (7, 15) Can't use a relative path to import a library in 'lib'. + // - Expected no errors but found 1 (fixes 0): + // - unused_import (7, 8) Unused import: 'a.dart'. + // + // Missing support for example files outside of `lib`. + 'avoid_relative_lib_imports', + + // avoid_web_libraries_in_flutter (linter) + // - Expected one error but found none (example 0). + // + // The example isn't being recognized as a flutter app. We might need to + // build a pubspec.yaml when analyzing flutter code. + 'avoid_web_libraries_in_flutter', + + // depend_on_referenced_packages (linter) + // - Expected one error but found none (example 0). + // + // The example doesn't generate the documented diagnostic. + 'depend_on_referenced_packages', + + // file_names (linter) + // - No example. + // + // There's no interesting file content to use as an example. We could have + // some placeholder content to get rid of the failure, but the documentation + // wouldn't be improved. + 'file_names', + + // prefer_inlined_adds_single (linter) + // - Expected one error but found none (example 1). + // + // Doesn't produce a lint for the second example, even though the analyzer + // does when the example is pasted into a file. + 'prefer_inlined_adds_single', + + // library_annotations (linter) + // - Expected an error with code library_annotations, found + // undefined_annotation (example 0). + // - Expected no errors but found 1 (fixes 0): + // - undefined_annotation (0, 18) Undefined name 'TestOn' used as an + // annotation. + // + // No mock 'test' package, no good library annotations in 'meta'. + 'library_annotations', + + // package_prefixed_library_names (linter) + // - Expected one error but found none (example 0). + // + // The lint does nothing, so no diagnostic is produced. I needs to be marked + // as 'removed'. + 'package_prefixed_library_names', + + // prefer_relative_imports (linter) + // No error range in example + // - Expected no errors but found 1 (example 0): + // - uri_does_not_exist (7, 29) Target of URI doesn't exist: + // 'package:my_package/bar.dart'. + // - Expected no errors but found 1 (fixes 0): + // - uri_does_not_exist (7, 10) Target of URI doesn't exist: 'bar.dart'. + // + // Need a way to specify the existance of files whose content is irrelevant. + // Either that or the example needs to include a minial file to refer to. + 'prefer_relative_imports', + + // public_member_api_docs (linter) + // - Expected one error but found none (example 0). + // + // The test file is in a basic workspace, so it can't have public API. I + // think we'd need to add a `pubspec.yaml` file to the example. + 'public_member_api_docs', + + // recursive_getters (linter) + // - Expected an error at 39, found 48 (example 0). + // + // The lint fires when the example is pasted into an empty file. + 'recursive_getters', + + // Missing a mock of `Expando` in `dart:core`. + 'extension_declares_instance_field', + + // deprecated_extends_function (analyzer) + // - Expected an error with code deprecated_subtype_of_function, found + // final_class_extended_outside_of_library (example 0). + // + // Probably needs a language override comment, but I don't know which + // version. + 'deprecated_extends_function', ]; /// The buffer to which validation errors are written. @@ -443,6 +468,7 @@ class DocumentationValidator { test.setUp(); await test.resolveTestFile(); var diagnostics = test.result.diagnostics; + var filteredDiagnostics = []; var errorCount = 0; var unneededIgnores = snippet.ignores.toList(); for (var diagnostic in diagnostics) { @@ -451,6 +477,7 @@ class DocumentationValidator { unneededIgnores.remove(diagnosticName); } else { errorCount++; + filteredDiagnostics.add(diagnostic); } } @@ -465,7 +492,7 @@ class DocumentationValidator { if (errorCount == 0) { _reportProblem('Expected one error but found none ($section $index).'); } else if (errorCount == 1) { - var diagnostic = diagnostics[0]; + var diagnostic = filteredDiagnostics[0]; if (diagnostic.diagnosticCode.lowerCaseName != codeName) { _reportProblem( 'Expected an error with code $codeName, ' diff --git a/pkg/linter/messages.yaml b/pkg/linter/messages.yaml index 019bd9fe0d9..a7351705676 100644 --- a/pkg/linter/messages.yaml +++ b/pkg/linter/messages.yaml @@ -361,6 +361,7 @@ LinterLintCode: diagnostic because a relative URI is used to import `a.dart`: ```dart + %ignore=uri_does_not_exist import [!'a.dart'!]; ``` @@ -369,6 +370,7 @@ LinterLintCode: Use a package import: ```dart + %ignore=uri_does_not_exist import 'package:p/a.dart'; ``` deprecatedDetails: |- @@ -3236,6 +3238,7 @@ LinterLintCode: produces this diagnostic because it imports `dart:html`: ```dart + %ignore=unused_import import [!'dart:html'!]; import 'package:flutter/material.dart'; @@ -3248,6 +3251,7 @@ LinterLintCode: If the package isn't intended to be a web plugin, then remove the import: ```dart + %ignore=unused_import import 'package:flutter/material.dart'; class C {} @@ -4016,13 +4020,13 @@ LinterLintCode: statement inside a `finally` block: ```dart - int f() { + void f(void Function() g) { try { - return 1; + g(); } catch (e) { print(e); } finally { - [!return 0;!] + [!return;!] } } ``` @@ -4033,9 +4037,9 @@ LinterLintCode: `finally` clause if the block is empty: ```dart - int f() { + void f(void Function() g) { try { - return 1; + g(); } catch (e) { print(e); } @@ -4046,13 +4050,13 @@ LinterLintCode: block: ```dart - int f() { + void f(void Function() g) { try { - return 1; + g(); } catch (e) { print(e); } - return 0; + return; } ``` deprecatedDetails: |- @@ -4325,7 +4329,8 @@ LinterLintCode: on the package `a`: ```dart - import 'package:a/a.dart'; + %ignore=uri_does_not_exist + import [!'package:a/a.dart'!]; ``` #### Common fixes @@ -5457,6 +5462,7 @@ LinterLintCode: top-level `src` directory: ```dart + %ignore=uri_does_not_exist import [!'package:ffi/src/allocation.dart'!]; ``` @@ -5466,6 +5472,7 @@ LinterLintCode: then import the public library that exports the public API: ```dart + %ignore=unused_import import 'package:ffi/ffi.dart'; ``` @@ -6551,6 +6558,7 @@ LinterLintCode: `ffiSupport` isn't a lower_case_with_underscores identifier: ```dart + %ignore=unused_import import 'package:ffi/ffi.dart' as [!ffiSupport!]; ``` @@ -6560,6 +6568,7 @@ LinterLintCode: convention: ```dart + %ignore=unused_import import 'package:ffi/ffi.dart' as ffi_support; ``` deprecatedDetails: |- @@ -6615,6 +6624,7 @@ LinterLintCode: it private: ```dart + %ignore=unused_element void _f(_C c) {} class _C {} @@ -11519,6 +11529,7 @@ LinterLintCode: invokes itself: ```dart + %ignore=unused_field class C { int _count = 0; @@ -16559,6 +16570,7 @@ LinterLintCode: used to set the value of the field `_f` and does no other work: ```dart + %ignore=unused_field class C { int _f = 0; @@ -16571,6 +16583,7 @@ LinterLintCode: Convert the method to a setter: ```dart + %ignore=unused_field class C { int _f = 0;