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

Remove all dynamic calls and properly enforce lint rule #2582

Merged
merged 7 commits into from
Mar 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions analysis_options.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ analyzer:
linter:
rules:
- always_declare_return_types
- avoid_dynamic_calls
- avoid_single_cascade_in_expression_statements
- avoid_unused_constructor_parameters
- annotate_overrides
Expand Down
1 change: 1 addition & 0 deletions analysis_options_presubmit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ analyzer:
linter:
rules:
- always_declare_return_types
- avoid_dynamic_calls
- avoid_single_cascade_in_expression_statements
- avoid_unused_constructor_parameters
- annotate_overrides
Expand Down
54 changes: 27 additions & 27 deletions lib/src/dartdoc_options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -343,32 +343,32 @@ class ToolConfiguration {
List<String> findCommand([String prefix = '']) {
List<String> command;
// If the command key is given, then it applies to all platforms.
var commandFrom = toolMap.containsKey('${prefix}command')
var commandFromKey = toolMap.containsKey('${prefix}command')
? '${prefix}command'
: '$prefix${Platform.operatingSystem}';
if (toolMap.containsKey(commandFrom)) {
if (toolMap[commandFrom].value is String) {
command = [toolMap[commandFrom].toString()];
if (toolMap.containsKey(commandFromKey)) {
var commandFrom = toolMap[commandFromKey] as YamlNode;
if (commandFrom.value is String) {
command = [commandFrom.toString()];
if (command[0].isEmpty) {
throw DartdocOptionError(
'Tool commands must not be empty. Tool $name command entry '
'"$commandFrom" must contain at least one path.');
'"$commandFromKey" must contain at least one path.');
}
} else if (toolMap[commandFrom] is YamlList) {
command = (toolMap[commandFrom] as YamlList)
.map<String>((node) => node.toString())
.toList();
} else if (commandFrom is YamlList) {
command =
commandFrom.map<String>((node) => node.toString()).toList();
if (command.isEmpty) {
throw DartdocOptionError(
'Tool commands must not be empty. Tool $name command entry '
'"$commandFrom" must contain at least one path.');
'"$commandFromKey" must contain at least one path.');
}
} else {
throw DartdocOptionError(
'Tool commands must be a path to an executable, or a list of '
'strings that starts with a path to an executable. '
'The tool $name has a $commandFrom entry that is a '
'${toolMap[commandFrom].runtimeType}');
'The tool $name has a $commandFromKey entry that is a '
'${commandFrom.runtimeType}');
}
}
return command;
Expand Down Expand Up @@ -598,16 +598,17 @@ abstract class DartdocOption<T> {
_OptionValueWithContext<Object> valueWithContext, String missingFilename);

/// Call [_onMissing] for every path that does not exist.
void _validatePaths(_OptionValueWithContext<dynamic> valueWithContext) {
void _validatePaths(_OptionValueWithContext<Object> valueWithContext) {
if (!mustExist) return;
assert(isDir || isFile);
List<String> resolvedPaths;
if (valueWithContext.value is String) {
var value = valueWithContext.value;
if (value is String) {
resolvedPaths = [valueWithContext.resolvedValue];
} else if (valueWithContext.value is List<String>) {
resolvedPaths = valueWithContext.resolvedValue.toList();
} else if (valueWithContext.value is Map<String, String>) {
resolvedPaths = valueWithContext.resolvedValue.values.toList();
} else if (value is List<String>) {
resolvedPaths = valueWithContext.resolvedValue as List;
} else if (value is Map<String, String>) {
resolvedPaths = (valueWithContext.resolvedValue as Map).values.toList();
} else {
assert(
false,
Expand Down Expand Up @@ -1120,19 +1121,18 @@ abstract class _DartdocFileOption<T> implements DartdocOption<T> {
_OptionValueWithContext<Object> _valueAtFromFile(Folder dir) {
var yamlFileData = _yamlAtDirectory(dir);
var contextPath = yamlFileData.canonicalDirectoryPath;
dynamic yamlData = yamlFileData.data ?? {};
Object yamlData = yamlFileData.data ?? {};
for (var key in keys) {
if (!yamlData.containsKey(key)) return null;
yamlData = yamlData[key] ?? {};
if (yamlData is Map && !yamlData.containsKey(key)) return null;
yamlData = (yamlData as Map)[key] ?? {};
}

dynamic returnData;
Object returnData;
if (_isListString) {
if (yamlData is YamlList) {
returnData = <String>[];
for (var item in yamlData) {
returnData.add(item.toString());
}
returnData = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, though it seems odd to me to omit <String>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String is inferred from the elements as the type of the list. This is listed in Effective Dart:

https://dart.dev/guides/language/effective-dart/design#dont-write-type-arguments-on-generic-invocations-that-are-inferred

for (var item in yamlData) item.toString(),
];
}
} else if (yamlData is YamlMap) {
// TODO(jcollins-g): This special casing is unfortunate. Consider
Expand Down Expand Up @@ -1700,7 +1700,7 @@ Future<List<DartdocOption<Object>>> createDartdocOptions(
help: 'Generate ONLY the docs for the Dart SDK.'),
DartdocOptionArgSynth<String>('sdkDir',
(DartdocSyntheticOption<String> option, Folder dir) {
if (!option.parent['sdkDocs'].valueAt(dir) &&
if (!(option.parent['sdkDocs'].valueAt(dir) as bool) &&
(option.root['topLevelPackageMeta'].valueAt(dir) as PackageMeta)
.requiresFlutter) {
String flutterRoot = option.root['flutterRoot'].valueAt(dir);
Expand Down
2 changes: 1 addition & 1 deletion lib/src/model/model_element.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1097,7 +1097,7 @@ abstract class ModelElement extends Canonicalization
}
if (params == null && element is FunctionTypedElement) {
if (_originalMember != null) {
params = (_originalMember as dynamic).parameters;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good one to get rid of. That might cover up some nasty bugs otherwise. Don't remember why that is there...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha it was a little weird.

params = (_originalMember as FunctionTypedElement).parameters;
} else {
params = (element as FunctionTypedElement).parameters;
}
Expand Down
8 changes: 6 additions & 2 deletions lib/src/package_meta.dart
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,12 @@ class _FilePackageMeta extends PubPackageMeta {

@override
bool get requiresFlutter =>
_pubspec['environment']?.containsKey('flutter') == true ||
_pubspec['dependencies']?.containsKey('flutter') == true;
_environment?.containsKey('flutter') == true ||
_dependencies?.containsKey('flutter') == true;

YamlMap /*?*/ get _environment => _pubspec['environment'];

YamlMap /*?*/ get _dependencies => _pubspec['environment'];

@override
File getReadmeContents() =>
Expand Down
24 changes: 12 additions & 12 deletions test/dartdoc_options_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ void main() {
dartdocOptionSetSynthetic.add(
DartdocOptionSyntheticOnly<List<String>>('vegetableLoader',
(DartdocSyntheticOption<List<String>> option, Folder dir) {
if (option.root['mySpecialInteger'].valueAt(dir) > 20) {
if ((option.root['mySpecialInteger'].valueAt(dir) as num) > 20) {
return <String>['existing.dart'];
} else {
return <String>['not_existing.dart'];
Expand All @@ -81,7 +81,7 @@ void main() {
dartdocOptionSetSynthetic.add(
DartdocOptionArgSynth<String>('nonCriticalFileOption',
(DartdocSyntheticOption<String> option, Folder dir) {
return option.root['vegetableLoader'].valueAt(dir).first;
return (option.root['vegetableLoader'].valueAt(dir) as List).first;
}, resourceProvider, optionIs: OptionKind.file));

dartdocOptionSetFiles = DartdocOptionSet('dartdoc', resourceProvider);
Expand Down Expand Up @@ -637,13 +637,13 @@ dartdoc:
equals(
'Field dartdoc.fileOptionList from ${path.canonicalize(dartdocOptionsTwo.path)}, set to [existing.dart, thing/that/does/not/exist], resolves to missing path: '
'"${path.joinAll([
path.canonicalize(secondDir.path),
'thing',
'that',
'does',
'not',
'exist'
])}"'));
path.canonicalize(secondDir.path),
'thing',
'that',
'does',
'not',
'exist'
])}"'));
// It doesn't matter that this fails.
expect(dartdocOptionSetFiles['nonCriticalFileOption'].valueAt(firstDir),
equals(path.joinAll([path.canonicalize(firstDir.path), 'whatever'])));
Expand All @@ -663,9 +663,9 @@ dartdoc:
equals(
'Field dartdoc.fileOption from ${path.canonicalize(dartdocOptionsTwo.path)}, set to not existing, resolves to missing path: '
'"${path.joinAll([
path.canonicalize(secondDir.path),
"not existing"
])}"'));
path.canonicalize(secondDir.path),
"not existing"
])}"'));
});

test('DartdocOptionSetFile works for directory options', () {
Expand Down
2 changes: 1 addition & 1 deletion test/end2end/dartdoc_integration_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ void main() {
});

test('missing a required file path prints a fatal-error', () async {
var outputLines = [];
var outputLines = <String>[];
var impossiblePath = path.join(dartdocPath, 'impossible');
await expectLater(
() => subprocessLauncher.runStreamed(
Expand Down
15 changes: 9 additions & 6 deletions tool/doc_packages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -124,17 +124,20 @@ Future<List<String>> _packageUrls(int page) {
return http
.get(Uri.parse('https://pub.dartlang.org/packages.json?page=$page'))
.then((response) {
return List<String>.from(json.decode(response.body)['packages']);
var decodedJson = json.decode(response.body) as Map;
return (decodedJson['packages'] as List).cast<String>();
});
}

Future<List<PackageInfo>> _getPackageInfos(List<String> packageUrls) {
var futures = packageUrls.map((String p) {
return http.get(Uri.parse(p)).then((response) {
var decodedJson = json.decode(response.body);
var decodedJson = json.decode(response.body) as Map;
String name = decodedJson['name'];
var versions = List<Version>.from(
decodedJson['versions'].map((v) => Version.parse(v)));
var versions = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming inference works properly for the list type here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, Version.parse returns a Version, and this is the only element (repeated) in the list, so inference will find Version.

for (var version in decodedJson['versions'] as List)
Version.parse(version as String),
];
return PackageInfo(name, Version.primary(versions));
});
}).toList();
Expand Down Expand Up @@ -216,8 +219,8 @@ Future<void> _exec(String command, List<String> args,
});
}

bool _isOldSdkConstraint(var pubspecInfo) {
var environment = pubspecInfo['environment'];
bool _isOldSdkConstraint(Map<String, dynamic> pubspecInfo) {
srawlins marked this conversation as resolved.
Show resolved Hide resolved
var environment = pubspecInfo['environment'] as Map;
if (environment != null) {
var sdk = environment['sdk'];
if (sdk != null) {
Expand Down
14 changes: 6 additions & 8 deletions tool/grind.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import 'package:dartdoc/src/package_meta.dart';
import 'package:grinder/grinder.dart';
import 'package:path/path.dart' as path;
import 'package:yaml/yaml.dart' as yaml;
import 'package:yaml/yaml.dart';

import 'subprocess_launcher.dart';

Expand Down Expand Up @@ -402,7 +403,8 @@ WarningsCollection jsonMessageIterableToWarnings(Iterable<Map> messageIterable,
if (message.containsKey('level') &&
message['level'] == 'WARNING' &&
message.containsKey('data')) {
warningTexts.add(message['data']['text']);
var data = message['data'] as Map;
warningTexts.add(data['text']);
}
}
return warningTexts;
Expand Down Expand Up @@ -1001,15 +1003,11 @@ Future<void> checkChangelogHasVersion() async {

String _getPackageVersion() {
var pubspec = File('pubspec.yaml');
dynamic yamlDoc;
if (pubspec.existsSync()) {
yamlDoc = yaml.loadYaml(pubspec.readAsStringSync());
}
if (yamlDoc == null) {
if (!pubspec.existsSync()) {
fail('Cannot find pubspec.yaml in ${Directory.current}');
}
var version = yamlDoc['version'];
return version;
var yamlDoc = yaml.loadYaml(pubspec.readAsStringSync()) as YamlMap;
return yamlDoc['version'];
}

@Task('Rebuild generated files')
Expand Down
3 changes: 2 additions & 1 deletion tool/subprocess_launcher.dart
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,8 @@ class SubprocessLauncher {
if (result.containsKey('message')) {
line = result['message'];
} else if (result.containsKey('data')) {
line = result['data']['text'];
var data = result['data'] as Map;
line = data['text'];
}
}
return line.split('\n');
Expand Down