From 7320da0d191a0918c308ce88448bb890ec2b6a1d Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Fri, 15 Dec 2023 04:04:28 +0000 Subject: [PATCH] linter: Add a PRESUBMIT check for example/all.yaml Work towards https://github.com/dart-lang/sdk/issues/53578 Change-Id: Ia07d999abc2fcf4b8195c9f7688799bc099a1d88 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/341385 Reviewed-by: Phil Quitslund Reviewed-by: Alexander Thomas Reviewed-by: Jonas Termansen Commit-Queue: Samuel Rawlins --- PRESUBMIT.py | 24 +++- pkg/linter/test/integration_test.dart | 70 +----------- pkg/linter/test/test_constants.dart | 19 +--- pkg/linter/tool/checks/check_all_yaml.dart | 126 +++++++++++++++++++++ pkg/linter/tool/machine.dart | 2 +- pkg/linter/tool/scorecard.dart | 2 +- pkg/linter/tool/since.dart | 2 +- pkg/linter/tool/util/path_utils.dart | 28 +++++ 8 files changed, 182 insertions(+), 91 deletions(-) create mode 100644 pkg/linter/tool/checks/check_all_yaml.dart create mode 100644 pkg/linter/tool/util/path_utils.dart diff --git a/PRESUBMIT.py b/PRESUBMIT.py index c49a42ede75..9b16b7f0284 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -298,14 +298,13 @@ def _CheckClangTidy(input_api, output_api): def _CheckAnalyzerFiles(input_api, output_api): """Run analyzer checks on source files.""" - # The first (and so far, only) check, is to verify the "error fix status" - # file. - relevant_files = [ + # Verify the "error fix status" file. + code_files = [ "pkg/analyzer/lib/src/error/error_code_values.g.dart", "pkg/linter/lib/src/rules.dart", ] - if any(f.LocalPath() in relevant_files for f in input_api.AffectedFiles()): + if any(f.LocalPath() in code_files for f in input_api.AffectedFiles()): args = [ "tools/sdks/dart-sdk/bin/dart", "pkg/analysis_server/tool/presubmit/verify_error_fix_status.dart", @@ -320,6 +319,23 @@ def _CheckAnalyzerFiles(input_api, output_api): long_text=stdout) ] + # Verify the linter's `example/all.yaml` file. + if any(f.LocalPath().startswith('pkg/linter/lib/src/rules') + for f in input_api.AffectedFiles()): + args = [ + "tools/sdks/dart-sdk/bin/dart", + "pkg/analysis_server/tool/checks/check_all_yaml.dart", + ] + stdout = input_api.subprocess.check_output(args).strip() + if not stdout: + return [] + + return [ + output_api.PresubmitError( + "The check_all_yaml linter tool revealed issues:", + long_text=stdout) + ] + # TODO(srawlins): Check more: # * "verify_sorted" for individual modified (not deleted) files in # Analyzer-team-owned directories. diff --git a/pkg/linter/test/integration_test.dart b/pkg/linter/test/integration_test.dart index 99315339d23..baae2510999 100644 --- a/pkg/linter/test/integration_test.dart +++ b/pkg/linter/test/integration_test.dart @@ -5,25 +5,13 @@ import 'dart:io'; import 'package:analyzer/src/lint/io.dart'; -import 'package:analyzer/src/lint/state.dart'; -import 'package:linter/src/analyzer.dart'; -import 'package:linter/src/rules.dart'; -import 'package:linter/src/utils.dart'; import 'package:test/test.dart'; -import 'package:yaml/yaml.dart'; +import '../tool/checks/check_all_yaml.dart'; import 'mocks.dart'; -import 'test_constants.dart'; void main() { - // ignore: unnecessary_lambdas group('integration', () { - coreTests(); - }); -} - -void coreTests() { - group('core', () { group('config', () { var currentOut = outSink; var collectingOut = CollectingSink(); @@ -40,61 +28,11 @@ void coreTests() { group('examples', () { test('all.yaml', () { - var src = readFile(pathRelativeToPackageRoot(['example', 'all.yaml'])); - - var options = _getOptionsFromString(src); - var configuredLints = - // ignore: cast_nullable_to_non_nullable - (options['linter'] as YamlMap)['rules'] as YamlList; - - // rules are sorted - expect( - configuredLints, orderedEquals(configuredLints.toList()..sort())); - - registerLintRules(); - - var registered = Analyzer.facade.registeredRules - .where((r) => - !r.state.isDeprecated && - !r.state.isInternal && - !r.state.isRemoved) - .map((r) => r.name); - - for (var l in configuredLints) { - if (!registered.contains(l)) { - printToConsole(l); - } + var errors = checkAllYaml(); + if (errors != null) { + fail(errors); } - - expect(configuredLints, unorderedEquals(registered)); }); }); }); } - -/// Provide the options found in [optionsSource]. -Map _getOptionsFromString(String optionsSource) { - var options = {}; - var doc = loadYamlNode(optionsSource); - - // Empty options. - if (doc is YamlScalar && doc.value == null) { - return options; - } - if (doc is! YamlMap) { - throw Exception( - 'Bad options file format (expected map, got ${doc.runtimeType})'); - } - doc.nodes.forEach((k, YamlNode v) { - Object? key; - if (k is YamlScalar) { - key = k.value; - } - if (key is! String) { - throw Exception('Bad options file format (expected String scope key, ' - 'got ${k.runtimeType})'); - } - options[key] = v; - }); - return options; -} diff --git a/pkg/linter/test/test_constants.dart b/pkg/linter/test/test_constants.dart index c8ef1717b96..6ca7ca0ed10 100644 --- a/pkg/linter/test/test_constants.dart +++ b/pkg/linter/test/test_constants.dart @@ -2,9 +2,7 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -import 'dart:io'; - -import 'package:path/path.dart' as path; +import '../tool/util/path_utils.dart'; final String integrationTestDir = pathRelativeToPackageRoot(['test_data', 'integration']); @@ -12,18 +10,3 @@ final String ruleTestDataDir = pathRelativeToPackageRoot(['test_data', 'rules']); final String ruleTestDir = pathRelativeToPackageRoot(['test', 'rules']); final String testConfigDir = pathRelativeToPackageRoot(['test', 'configs']); - -List get _scriptPathParts => - path.split(path.dirname(path.fromUri(Platform.script.path))); - -String pathRelativeToPackageRoot(Iterable parts) { - var pathParts = _scriptPathParts; - pathParts.replaceRange(pathParts.length - 1, pathParts.length, parts); - return path.joinAll(pathParts); -} - -String pathRelativeToPkgDir(Iterable parts) { - var pathParts = _scriptPathParts; - pathParts.replaceRange(pathParts.length - 2, pathParts.length, parts); - return path.joinAll(pathParts); -} diff --git a/pkg/linter/tool/checks/check_all_yaml.dart b/pkg/linter/tool/checks/check_all_yaml.dart new file mode 100644 index 00000000000..a846f5bd6dc --- /dev/null +++ b/pkg/linter/tool/checks/check_all_yaml.dart @@ -0,0 +1,126 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'dart:io'; + +import 'package:analyzer/src/lint/io.dart'; +import 'package:analyzer/src/lint/state.dart'; +import 'package:linter/src/analyzer.dart'; +import 'package:linter/src/rules.dart'; +import 'package:yaml/yaml.dart'; + +import '../util/path_utils.dart'; + +/// Checks the 'example/all.yaml' file for correctness. +/// +/// Prints any errors. +void main() { + var errors = checkAllYaml(); + if (errors != null) { + // ignore: avoid_print + print(errors); + exitCode = 1; + } +} + +/// Checks the 'example/all.yaml' file for correctness, returning a String if +/// there are errors, and `null` otherwise. +String? checkAllYaml() { + var allYamlPath = pathRelativeToPackageRoot(['example', 'all.yaml']); + var src = readFile(allYamlPath); + + var options = _getOptionsFromString(src); + var linterSection = options['linter'] as YamlMap?; + if (linterSection == null) { + return "Error: '$allYamlPath' does not have a 'linter' section."; + } + + var configuredRules = (linterSection['rules'] as YamlList?)?.cast(); + if (configuredRules == null) { + return "Error: '$allYamlPath' does not have a 'rules' section."; + } + + var sortedRules = configuredRules.toList()..sort(); + + for (var i = 0; i < configuredRules.length; i++) { + if (configuredRules[i] != sortedRules[i]) { + return "Error: Rules in '$allYamlPath' are not sorted alphabetically, " + "starting at '${configuredRules[i]}'."; + } + } + + registerLintRules(); + + var registeredRules = Analyzer.facade.registeredRules + .where((r) => + !r.state.isDeprecated && !r.state.isInternal && !r.state.isRemoved) + .map((r) => r.name); + + var extraRules = []; + var missingRules = []; + + for (var rule in configuredRules) { + if (!registeredRules.contains(rule)) { + extraRules.add(rule); + } + } + for (var rule in registeredRules) { + if (!configuredRules.contains(rule)) { + missingRules.add(rule); + } + } + + if (extraRules.isEmpty && missingRules.isEmpty) { + return null; + } + + var errors = StringBuffer(); + if (extraRules.isNotEmpty) { + errors.writeln('Found unknown (or deprecated/removed) rules:'); + for (var rule in extraRules) { + errors.writeln('- $rule'); + } + } + if (missingRules.isNotEmpty) { + errors.writeln('Missing rules:'); + for (var rule in missingRules) { + errors.writeln('- $rule'); + } + } + return errors.toString(); +} + +/// Provides the options found in [optionsSource]. +Map _getOptionsFromString(String optionsSource) { + var options = {}; + var doc = loadYamlNode(optionsSource); + + // Empty options. + if (doc is YamlScalar && doc.value == null) { + return options; + } + if (doc is! YamlMap) { + throw Exception( + 'Bad options file format (expected map, got ${doc.runtimeType})'); + } + doc.nodes.forEach((k, YamlNode v) { + if (k is! YamlScalar) { + throw YamlException( + 'Bad options file format (expected YamlScalar key, got ' + "'${k.runtimeType}'", + v.span, + ); + } + var key = k.value; + if (key is! String) { + throw YamlException( + 'Bad options file format (expected String key, got ' + "'${key.runtimeType})'", + v.span, + ); + } + options[key] = v; + }); + return options; +} diff --git a/pkg/linter/tool/machine.dart b/pkg/linter/tool/machine.dart index 2b0e667cb35..3caa0da0c73 100644 --- a/pkg/linter/tool/machine.dart +++ b/pkg/linter/tool/machine.dart @@ -13,7 +13,7 @@ import 'package:linter/src/rules.dart'; import 'package:linter/src/utils.dart'; import 'package:yaml/yaml.dart'; -import '../test/test_constants.dart'; +import '../tool/util/path_utils.dart'; import 'since.dart'; import 'util/score_utils.dart' as score_utils; diff --git a/pkg/linter/tool/scorecard.dart b/pkg/linter/tool/scorecard.dart index 9d2b7926c84..531e1de6f12 100644 --- a/pkg/linter/tool/scorecard.dart +++ b/pkg/linter/tool/scorecard.dart @@ -13,7 +13,7 @@ import 'package:linter/src/analyzer.dart'; import 'package:linter/src/rules.dart'; import 'package:linter/src/utils.dart'; -import '../test/test_constants.dart'; +import '../tool/util/path_utils.dart'; import 'crawl.dart'; import 'parse.dart'; import 'since.dart'; diff --git a/pkg/linter/tool/since.dart b/pkg/linter/tool/since.dart index e786723f0c1..4807ecd1027 100644 --- a/pkg/linter/tool/since.dart +++ b/pkg/linter/tool/since.dart @@ -7,7 +7,7 @@ import 'dart:io'; import 'package:yaml/yaml.dart'; -import '../test/test_constants.dart'; +import '../tool/util/path_utils.dart'; import 'changelog.dart'; import 'crawl.dart'; diff --git a/pkg/linter/tool/util/path_utils.dart b/pkg/linter/tool/util/path_utils.dart new file mode 100644 index 00000000000..27383d14b14 --- /dev/null +++ b/pkg/linter/tool/util/path_utils.dart @@ -0,0 +1,28 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'dart:io'; + +import 'package:path/path.dart' as path; + +List get _packageRoot { + var parts = path.split(path.dirname(path.fromUri(Platform.script.path))); + while (parts.last != 'linter') { + parts.removeLast(); + if (parts.isEmpty) { + throw StateError("Script is not located inside a 'linter' directory? " + "'${Platform.script.path}'"); + } + } + return parts; +} + +String pathRelativeToPackageRoot(Iterable parts) => + path.joinAll([..._packageRoot, ...parts]); + +String pathRelativeToPkgDir(Iterable parts) { + var pathParts = _packageRoot; + pathParts.replaceRange(pathParts.length - 1, pathParts.length, parts); + return path.joinAll(pathParts); +}