Skip to content

Commit

Permalink
analyzer: Convert applyOptions extension method into AnalysisOptionsB…
Browse files Browse the repository at this point in the history
…uilder builder.

Previously, the fields in an AnalysisOptionsImpl were primarily
arranged by an extension method, declared externally, called
`applyOptions`. It seemed very odd that this wasn't just a method on
AnalysisOptionsImpl; the "Impl" class is already private.

But it turns out that an AnalysisOptionsImpl is _mostly_ not mutated
in practice; the class wants to be immutable, with final fields.

In this change, I make AnalysisOptionsImpl mostly immutable, and
convert the `applyOptions` extension method, and all of its helper
code, into a builder class, AnalysisOptionsBuilder. While we don't
use a lot of builders in analyzer packages, this is a much more common
and idiomatic pattern for setting up complicated data, multiple values,
and then finalizing it all into an object that will not be changed
again during it's lifetime.

One big benefit of this refactoring is that most fields in AnalysisOptionsImpl are now final:

* sourceLanguageConstraint
* errorProcessors
* excludePatterns
* file
* strictCasts
* strictInference
* strictRawTypes
* chromeOsManifestChecks
* codeStyleOptions
* formatterOptions
* unignorableNames

Whereas before, they _all_ had to be mutable, as `applyOptions`, the
primary mechanism for setting up an AnalysisOptionsImpl, had to able
to write any field.

Other changes:

* Flip the instantiation of AnalysisOptionsImpl and CodeStyleOptions;
  now CodeStyleOptions is instatiated first, and AnalysisOptionsImpl
  sets itself as `codeStyleOptions.options`.
* Remove the private, deprecated, `applyToAnalysisOptions` function.

Change-Id: I6595d88aa5721e4f9a8b2b987482f9f43d27efd1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/390660
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
  • Loading branch information
srawlins authored and Commit Queue committed Oct 17, 2024
1 parent 640ad14 commit e29aa0c
Show file tree
Hide file tree
Showing 18 changed files with 614 additions and 542 deletions.
255 changes: 0 additions & 255 deletions pkg/analyzer/lib/src/analysis_options/apply_options.dart

This file was deleted.

6 changes: 3 additions & 3 deletions pkg/analyzer/lib/src/analysis_options/code_style_options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import 'package:analyzer/dart/ast/ast.dart';
/// The concrete implementation of [CodeStyleOptions].
class CodeStyleOptionsImpl implements CodeStyleOptions {
/// The analysis options that owns this instance.
final AnalysisOptions _options;
late final AnalysisOptions options;

@override
final bool useFormatter;

CodeStyleOptionsImpl(this._options, {required this.useFormatter});
CodeStyleOptionsImpl({required this.useFormatter});

@override
bool get addTrailingCommas => _isLintEnabled('require_trailing_commas');
Expand Down Expand Up @@ -77,7 +77,7 @@ class CodeStyleOptionsImpl implements CodeStyleOptions {
}

/// Returns whether the lint rule with the given [name] is enabled.
bool _isLintEnabled(String name) => _options.isLintEnabled(name);
bool _isLintEnabled(String name) => options.isLintEnabled(name);

/// Returns the preferred lint quote, otherwise `null`.
String? _lintQuote() => _isLintEnabled('prefer_single_quotes')
Expand Down
45 changes: 23 additions & 22 deletions pkg/analyzer/lib/src/dart/analysis/context_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import 'package:analyzer/dart/analysis/declared_variables.dart';
import 'package:analyzer/file_system/file_system.dart';
import 'package:analyzer/file_system/physical_file_system.dart';
import 'package:analyzer/src/analysis_options/analysis_options_provider.dart';
import 'package:analyzer/src/analysis_options/apply_options.dart';
import 'package:analyzer/src/context/builder.dart' show EmbedderYamlLocator;
import 'package:analyzer/src/context/packages.dart';
import 'package:analyzer/src/dart/analysis/analysis_options_map.dart';
Expand Down Expand Up @@ -119,20 +118,19 @@ class ContextBuilderImpl implements ContextBuilder {
var optionsFile = contextRoot.optionsFile;
var sourceFactory = workspace.createSourceFactory(sdk, summaryData);

var analysisOptionsMap =
// If there's an options file defined (as, e.g. passed into the
// AnalysisContextCollection), use a shared options map based on it.
(definedOptionsFile && optionsFile != null)
? AnalysisOptionsMap.forSharedOptions(_getAnalysisOptions(
contextRoot,
optionsFile,
sourceFactory,
sdk,
updateAnalysisOptions2))
// Else, create one from the options file mappings stored in the
// context root.
: _createOptionsMap(
contextRoot, sourceFactory, updateAnalysisOptions2, sdk);
AnalysisOptionsMap analysisOptionsMap;
// If there's an options file defined (as, e.g. passed into the
// AnalysisContextCollection), use a shared options map based on it.
if (definedOptionsFile && optionsFile != null) {
analysisOptionsMap = AnalysisOptionsMap.forSharedOptions(
_getAnalysisOptions(contextRoot, optionsFile, sourceFactory, sdk,
updateAnalysisOptions2));
} else {
// Otherwise, create one from the options file mappings stored in the
// context root.
analysisOptionsMap = _createOptionsMap(
contextRoot, sourceFactory, updateAnalysisOptions2, sdk);
}

var analysisContext =
DriverBasedAnalysisContext(resourceProvider, contextRoot);
Expand Down Expand Up @@ -204,9 +202,9 @@ class ContextBuilderImpl implements ContextBuilder {
(contextRoot as ContextRootImpl).optionsFileMap.entries;
for (var entry in optionsMappings) {
var file = entry.value;
var options = AnalysisOptionsImpl(file: file);
var optionsYaml = provider.getOptionsFromFile(file);
options.applyOptions(optionsYaml);
var options = AnalysisOptionsImpl.fromYaml(
file: file, optionsMap: provider.getOptionsFromFile(file));

_optionsMap.add(entry.key, options);
}

Expand Down Expand Up @@ -300,17 +298,20 @@ class ContextBuilderImpl implements ContextBuilder {
required DartSdk sdk})?
updateAnalysisOptions,
) {
var options = AnalysisOptionsImpl(file: optionsFile);
AnalysisOptionsImpl? options;

if (optionsFile != null) {
try {
var provider = AnalysisOptionsProvider(sourceFactory);
var optionsMap = provider.getOptionsFromFile(optionsFile);
options.applyOptions(optionsMap);
options = AnalysisOptionsImpl.fromYaml(
optionsMap: provider.getOptionsFromFile(optionsFile),
file: optionsFile,
);
} catch (e) {
// ignore
// Ignore exception.
}
}
options ??= AnalysisOptionsImpl(file: optionsFile);

var pubspecFile = _findPubspecFile(contextRoot);
if (pubspecFile != null) {
Expand Down
8 changes: 4 additions & 4 deletions pkg/analyzer/lib/src/dart/analysis/context_locator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import 'package:analyzer/file_system/file_system.dart';
import 'package:analyzer/file_system/physical_file_system.dart'
show PhysicalResourceProvider;
import 'package:analyzer/src/analysis_options/analysis_options_provider.dart';
import 'package:analyzer/src/analysis_options/apply_options.dart';
import 'package:analyzer/src/context/packages.dart';
import 'package:analyzer/src/dart/analysis/context_root.dart';
import 'package:analyzer/src/generated/engine.dart';
Expand Down Expand Up @@ -543,9 +542,10 @@ class ContextLocatorImpl implements ContextLocator {
var provider =
AnalysisOptionsProvider(workspace.createSourceFactory(null, null));

var options = AnalysisOptionsImpl(file: optionsFile);
var optionsYaml = provider.getOptionsFromFile(optionsFile);
options.applyOptions(optionsYaml);
var options = AnalysisOptionsImpl.fromYaml(
file: optionsFile,
optionsMap: provider.getOptionsFromFile(optionsFile),
);

return options.enabledLegacyPluginNames.toSet();
} catch (_) {
Expand Down
Loading

0 comments on commit e29aa0c

Please sign in to comment.