Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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
2 changes: 0 additions & 2 deletions lib/analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
analyzer:
plugins:
- custom_lint
exclude:
# General generated files
- "**/*.g.dart"
Expand Down
14 changes: 12 additions & 2 deletions lib/main.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import 'package:analysis_server_plugin/plugin.dart';
import 'package:analysis_server_plugin/registry.dart';
import 'package:analyzer/analysis_rule/analysis_rule.dart';
import 'package:solid_lints/src/common/constants.dart';
import 'package:solid_lints/src/common/parameter_parser/analysis_options_loader.dart';
import 'package:solid_lints/src/lints/avoid_debug_print_in_release/avoid_debug_print_in_release_rule.dart';
import 'package:solid_lints/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule.dart';
Expand Down Expand Up @@ -30,6 +32,8 @@ import 'package:solid_lints/src/lints/prefer_last/prefer_last_rule.dart';
import 'package:solid_lints/src/lints/prefer_match_file_name/prefer_match_file_name_rule.dart';
import 'package:solid_lints/src/lints/proper_super_calls/proper_super_calls_rule.dart';
import 'package:solid_lints/src/lints/use_nearest_context/use_nearest_context_rule.dart';
import 'package:solid_lints/src/models/proxy_analysis_rule.dart';
import 'package:solid_lints/src/models/proxy_multi_analysis_rule.dart';
import 'package:solid_lints/src/models/rule_with_fixes.dart';

/// The entry point for the Solid Lints analyser server plugin.
Expand All @@ -44,7 +48,7 @@ final plugin = SolidLintsPlugin();
/// by the Dart analyzer during code analysis.
class SolidLintsPlugin extends Plugin {
@override
String get name => 'solid_lints';
String get name => kPluginName;

@override
void register(PluginRegistry registry) {
Expand Down Expand Up @@ -83,7 +87,13 @@ class SolidLintsPlugin extends Plugin {
];

for (final lintRule in lintRules) {
registry.registerLintRule(lintRule);
final ruleToRegister = switch (lintRule) {
MultiAnalysisRule() => ProxyMultiAnalysisRule(lintRule, analysisLoader),
AnalysisRule() => ProxyAnalysisRule(lintRule, analysisLoader),
};
Comment thread
solid-illiaaihistov marked this conversation as resolved.
Comment thread
solid-illiaaihistov marked this conversation as resolved.

registry.registerWarningRule(ruleToRegister);

if (lintRule is RuleWithFixes) {
for (final entry in (lintRule as RuleWithFixes).fixesForCodes) {
registry.registerFixForRule(entry.key, entry.value);
Expand Down
2 changes: 2 additions & 0 deletions lib/src/common/constants.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/// Shared constants for the solid_lints package.
const kPluginName = 'solid_lints';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be better if we made it into a static class instead, and hungarian notation is kinda redundant too

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done!

77 changes: 34 additions & 43 deletions lib/src/common/parameter_parser/analysis_options_loader.dart
Original file line number Diff line number Diff line change
@@ -1,17 +1,31 @@
import 'package:analyzer/analysis_rule/rule_context.dart';
import 'package:analyzer/file_system/file_system.dart';
import 'package:analyzer/file_system/physical_file_system.dart';
import 'package:solid_lints/src/common/parameter_parser/analysis_options_parser.dart';
import 'package:solid_lints/src/common/parameter_parser/cached_package_rules.dart';
import 'package:yaml/yaml.dart';
import 'package:solid_lints/src/common/parameter_parser/package_config_resolver.dart';

/// Loads and parses analysis options from a Dart project's YAML file.
class AnalysisOptionsLoader {
final ResourceProvider _resourceProvider;
final Map<String, CachedPackageRules> _rulesCache = {};

/// Creates an instance of [AnalysisOptionsLoader]
AnalysisOptionsLoader({ResourceProvider? resourceProvider})
: _resourceProvider = resourceProvider ?? PhysicalResourceProvider.INSTANCE;


late final AnalysisOptionsParser _parser;

/// Creates an instance of [AnalysisOptionsLoader].
AnalysisOptionsLoader({
ResourceProvider? resourceProvider,
AnalysisOptionsParser? parser,
}) : _resourceProvider =
resourceProvider ?? PhysicalResourceProvider.INSTANCE {
_parser = parser ??
AnalysisOptionsParser(
_resourceProvider,
PackageConfigResolver(_resourceProvider),
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can avoid the late keyword here:

Suggested change
late final AnalysisOptionsParser _parser;
/// Creates an instance of [AnalysisOptionsLoader].
AnalysisOptionsLoader({
ResourceProvider? resourceProvider,
AnalysisOptionsParser? parser,
}) : _resourceProvider =
resourceProvider ?? PhysicalResourceProvider.INSTANCE {
_parser = parser ??
AnalysisOptionsParser(
_resourceProvider,
PackageConfigResolver(_resourceProvider),
);
}
final AnalysisOptionsParser _parser;
/// Creates an instance of [AnalysisOptionsLoader].
factory AnalysisOptionsLoader({
ResourceProvider? resourceProvider,
AnalysisOptionsParser? parser,
}) {
final resolvedResourceProvider =
resourceProvider ?? PhysicalResourceProvider.INSTANCE;
final resolvedParser =
parser ??
AnalysisOptionsParser(
resolvedResourceProvider,
PackageConfigResolver(resolvedResourceProvider),
);
return AnalysisOptionsLoader._(
resourceProvider: resolvedResourceProvider,
parser: resolvedParser,
);
}
AnalysisOptionsLoader._({
required ResourceProvider resourceProvider,
required AnalysisOptionsParser parser,
}) : _resourceProvider = resourceProvider,
_parser = parser;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done!


/// Gets the options for a specific rule by its name.
Map<String, Object?>? getRuleOptions(RuleContext context, String ruleName) =>
Expand All @@ -36,6 +50,17 @@ class AnalysisOptionsLoader {
return _rulesCache[yamlPath]?.rules[ruleName];
}

/// Checks if a rule is explicitly disabled.
bool isRuleDisabled(RuleContext context, String ruleName) =>
_withNearestAnalysisOptionsFilePathForContext<bool>(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like type can be inferred here

Suggested change
_withNearestAnalysisOptionsFilePathForContext<bool>(
_withNearestAnalysisOptionsFilePathForContext(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done!

context,
(path) {
_loadRulesOptionsIfNewer(path);
return _rulesCache[path]?.disabledRules.contains(ruleName) ?? false;
},
) ??
false;

/// Loads lint rules from the analysis options file for all rules
/// using the provided [RuleContext].
void loadRulesOptionsFromContext(RuleContext context) =>
Expand Down Expand Up @@ -66,18 +91,19 @@ class AnalysisOptionsLoader {
return;
}

final rules = _getRules(analysisOptionsFile);
final rulesData = _parser.parse(analysisOptionsFile);
_rulesCache[yamlPath] = CachedPackageRules(
modificationStamp: modificationStamp,
rules: rules,
rules: rulesData.rules,
disabledRules: rulesData.disabledRules,
);
}

String? _findNearestAnalysisOptionsFilePath(String startDirectoryPath) {
final pathContext = _resourceProvider.pathContext;
String currentDirectoryPath = startDirectoryPath;
var currentDirectoryPath = startDirectoryPath;

while (true) {
while (currentDirectoryPath.isNotEmpty) {
final candidatePath = pathContext.join(
currentDirectoryPath,
'analysis_options.yaml',
Expand All @@ -97,39 +123,4 @@ class AnalysisOptionsLoader {

return null;
}
Comment thread
solid-illiaaihistov marked this conversation as resolved.

Map<String, Map<String, Object?>> _getRules(File? analysisOptionsFile) {
if (analysisOptionsFile == null || !analysisOptionsFile.exists) {
return {};
}

Object? yaml;
try {
final optionsString = analysisOptionsFile.readAsStringSync();
yaml = loadYaml(optionsString);
} catch (_) {
return {};
}

Object? rawDiagnostics;
if (yaml case {'solid_lints': {'diagnostics': final diagnostics}}) {
rawDiagnostics = diagnostics;
} else if (yaml case {
'plugins': {'solid_lints': {'diagnostics': final diagnostics}},
}) {
rawDiagnostics = diagnostics;
}

if (rawDiagnostics is! Map) return {};

return {
for (final entry in rawDiagnostics.entries)
if (entry.key is String && entry.value is Map)
entry.key as String: <String, Object?>{
for (final optionEntry in (entry.value as Map).entries)
if (optionEntry.key is String)
optionEntry.key as String: optionEntry.value,
},
};
}
}
211 changes: 211 additions & 0 deletions lib/src/common/parameter_parser/analysis_options_parser.dart

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have a lot of cases like:

for (final MapEntry(:key, ...) in ...) 
  if (key is String)
    ...

we should extract an extension

extension on Map<T, K> {
  Map<U, K> whereKeyType<U>();
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
import 'package:analyzer/file_system/file_system.dart';
import 'package:solid_lints/src/common/constants.dart';
import 'package:solid_lints/src/common/parameter_parser/package_config_resolver.dart';
import 'package:solid_lints/src/common/parameter_parser/rules_data.dart';
import 'package:yaml/yaml.dart';

/// Parser for analysis_options.yaml files to extract RulesData.
class AnalysisOptionsParser {
final ResourceProvider _resourceProvider;
final PackageConfigResolver _packageConfigResolver;

/// Creates a new instance of [AnalysisOptionsParser].
AnalysisOptionsParser(this._resourceProvider, this._packageConfigResolver);

/// Parses the given [analysisOptionsFile] and resolves its imports
/// to return [RulesData].
RulesData parse(File? analysisOptionsFile) {
return _parseWithSeen(analysisOptionsFile, {});
}

RulesData _parseWithSeen(File? analysisOptionsFile, Set<String> seenPaths) {
if (analysisOptionsFile == null || !analysisOptionsFile.exists) {
return const RulesData.empty();
}

final path = analysisOptionsFile.path;
if (seenPaths.contains(path)) {
return const RulesData.empty();
}

final yaml = _parseYaml(analysisOptionsFile);
if (yaml == null) {
return const RulesData.empty();
}

final mergedRules = <String, Map<String, Object?>>{};
final disabledRules = <String>{};

final nextSeenPaths = {...seenPaths, path};
_resolveAndMergeIncludes(
analysisOptionsFile,
yaml,
nextSeenPaths,
mergedRules,
disabledRules,
);
_parseRuleOptions(yaml, mergedRules, disabledRules);
_parseSuppressedErrors(yaml, mergedRules, disabledRules);

return RulesData(rules: mergedRules, disabledRules: disabledRules);
}
Comment thread
solid-illiaaihistov marked this conversation as resolved.

Map<String, Object?>? _parseYaml(File file) {
try {
final optionsString = file.readAsStringSync();
final parsed = loadYaml(optionsString);
if (parsed is! Map) return null;
return _toStandardMap(parsed);
} catch (_) {
return null;
}
}

Map<String, Object?> _toStandardMap(Map<dynamic, dynamic> map) {
return {
for (final entry in map.entries)
if (entry.key is String)
entry.key as String: _toStandardType(entry.value),
};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Map<String, Object?> _toStandardMap(Map<dynamic, dynamic> map) {
return {
for (final entry in map.entries)
if (entry.key is String)
entry.key as String: _toStandardType(entry.value),
};
}
Map<String, Object?> _toStandardMap(Map<dynamic, dynamic> map) => {
for (final MapEntry(:key, :value) in map.entries)
if (key is String) key: _toStandardType(value),
};

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done!


Object? _toStandardType(Object? value) => switch (value) {
Map() => _toStandardMap(value),
Iterable() => value.map(_toStandardType).toList(),
_ => value,
};

void _resolveAndMergeIncludes(
File baseFile,
Map<String, Object?> yaml,
Set<String> seenPaths,
Map<String, Map<String, Object?>> mergedRules,
Set<String> disabledRules,
) {
final includeOption = yaml['include'];
if (includeOption is! String) return;

final includedFile = _resolveIncludedFile(baseFile, includeOption);
if (includedFile == null) return;

final includedData = _parseWithSeen(includedFile, seenPaths);
mergedRules.addAll(includedData.rules);
disabledRules.addAll(includedData.disabledRules);
}
Comment thread
solid-illiaaihistov marked this conversation as resolved.

File? _resolveIncludedFile(File baseFile, String includePath) {
final pathContext = _resourceProvider.pathContext;
if (includePath.startsWith('package:')) {
final resolvedPath = _packageConfigResolver.resolvePackageUri(
baseFile.path,
includePath,
);
if (resolvedPath != null) {
return _resourceProvider.getFile(resolvedPath);
}
return null;
}

final baseDir = pathContext.dirname(baseFile.path);
final resolvedPath = pathContext.join(baseDir, includePath);
return _resourceProvider.getFile(resolvedPath);
}

void _parseRuleOptions(
Map<String, Object?> yaml,
Map<String, Map<String, Object?>> mergedRules,
Set<String> disabledRules,
) {
final rawDiagnostics = _extractDiagnostics(yaml);
if (rawDiagnostics is! Map) return;

for (final entry in rawDiagnostics.entries) {
final key = entry.key;
if (key is! String) continue;

final ruleName = key;
final value = entry.value;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (final entry in rawDiagnostics.entries) {
final key = entry.key;
if (key is! String) continue;
final ruleName = key;
final value = entry.value;
for (final MapEntry(key: ruleName, :value) in rawDiagnostics.entries) {
if (ruleName is! String) continue;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done!


if (value is Map) {
final existingOptions = mergedRules[ruleName] ?? {};
mergedRules[ruleName] = <String, Object?>{
...existingOptions,
for (final optionEntry in value.entries)
if (optionEntry.key is String)
optionEntry.key as String: optionEntry.value,
};
disabledRules.remove(ruleName);
} else if (value is bool) {
if (value) {
disabledRules.remove(ruleName);
} else {
mergedRules.remove(ruleName);
disabledRules.add(ruleName);
}
}
Comment thread
solid-illiaaihistov marked this conversation as resolved.
Outdated
Comment thread
solid-illiaaihistov marked this conversation as resolved.
}
}

Object? _extractDiagnostics(Map<String, Object?> yaml) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. We can make it linear:
  Object? _extractDiagnostics(Map<String, Object?> yaml) {
    final pluginConfig = yaml[kPluginName];
    if (pluginConfig is Map) return pluginConfig['diagnostics'];

    final pluginsConfig = yaml['plugins'];
    if (pluginsConfig is! Map) return null;

    final pluginSubConfig = pluginsConfig[kPluginName];
    if (pluginSubConfig is Map) return pluginSubConfig['diagnostics'];

    return null;
  }
  1. These two pairs are basically the same, we can probably extract them:
    final pluginConfig = yaml[kPluginName];
    if (pluginConfig is Map) return pluginConfig['diagnostics'];
    final pluginSubConfig = pluginsConfig[kPluginName];
    if (pluginSubConfig is Map) return pluginSubConfig['diagnostics'];

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We can eliminate the code duplication by iterating over the potential configuration sources in a loop. Here is what I propose:

Object? _extractDiagnostics(Map<String, Object?> yaml) {
    final sources = [yaml, yaml['plugins']];
    for (final source in sources) {
      if (source is! Map) continue;
      final config = source[SolidLintsConstants.pluginName];
      if (config is Map) return config['diagnostics'];
    }

    return null;
  }

final pluginConfig = yaml[kPluginName];
if (pluginConfig is Map) {
return pluginConfig['diagnostics'];
}

final pluginsConfig = yaml['plugins'];
if (pluginsConfig is Map) {
final pluginSubConfig = pluginsConfig[kPluginName];
if (pluginSubConfig is Map) {
return pluginSubConfig['diagnostics'];
}
}

return null;
}

/// Parses rule suppression configured under `analyzer: errors:`.
///
/// By default, when a user excludes/ignores a rule using the IDE
/// quick fix or standard settings, the Dart analysis server appends
/// the following to `analysis_options.yaml`:
///
/// ```yaml
/// analyzer:
/// errors:
/// solid_lints/rule_name: ignore
/// ```
///
/// To respect this standard mechanism and prevent the plugin rules
/// from running, we parse this section and add suppressed rules
/// to [disabledRules].
void _parseSuppressedErrors(
Map<String, Object?> yaml,
Map<String, Map<String, Object?>> mergedRules,
Set<String> disabledRules,
) {
final analyzer = yaml['analyzer'];
if (analyzer is! Map) return;

final errors = analyzer['errors'];
if (errors is! Map) return;

const pluginPrefix = '$kPluginName/';

for (final entry in errors.entries) {
final key = entry.key;
if (key is! String) continue;

final errorValue = entry.value;
final ruleName = key.startsWith(pluginPrefix)
? key.substring(pluginPrefix.length)
: key;

if (errorValue == 'ignore') {
mergedRules.remove(ruleName);
disabledRules.add(ruleName);
} else {
disabledRules.remove(ruleName);
}
Comment thread
solid-illiaaihistov marked this conversation as resolved.
Outdated
}
}
Comment thread
solid-illiaaihistov marked this conversation as resolved.
}
4 changes: 4 additions & 0 deletions lib/src/common/parameter_parser/cached_package_rules.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@ class CachedPackageRules {
/// Cached rules options by rule name for the package
final Map<String, Map<String, Object?>> rules;

/// Rules that are explicitly disabled
final Set<String> disabledRules;

/// Creates an instance of [CachedPackageRules]
const CachedPackageRules({
required this.modificationStamp,
required this.rules,
required this.disabledRules,
});
}
Loading
Loading