Skip to content

Commit

Permalink
Embedded option processing fixes (#25115).
Browse files Browse the repository at this point in the history 
* Updates error filter and linter configuration to play nice with embedded options.
  * Fixes embedded lint config (#25115).
* Improves support for (and tests of) options file deletions.
* Pulls in linter with untyped options support (needed for processing merged options).

Unblocks: flutter/flutter#624.

R=brianwilkerson@google.com

Review URL: https://codereview.chromium.org/1503353002 .
  • Loading branch information
@pq
pq committed Dec 8, 2015
1 parent 67306ae commit 1e4cea3
Show file tree
Hide file tree
Showing 4 changed files with 223 additions and 75 deletions.
2 changes: 1 addition & 1 deletion DEPS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ vars = {
"intl_rev": "@32047558bd220a53c1f4d93a26d54b83533b1475",
"jinja2_rev": "@2222b31554f03e62600cd7e383376a7c187967a1",
"json_rpc_2_tag": "@1.1.1",
"linter_rev": "@e5281475126efdc556c3eba6a8b6683fd814b033",
"linter_rev": "@97a27c37e549510966c1f5256f84586152ba6371",
"logging_rev": "@85d83e002670545e9039ad3985f0018ab640e597",
"markdown_rev": "@4aaadf3d940bb172e1f6285af4d2b1710d309982",
"matcher_tag": "@0.12.0",
Expand Down
43 changes: 23 additions & 20 deletions pkg/analysis_server/lib/src/context_manager.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ class ContextManagerImpl implements ContextManager {
*/
void processOptionsForContext(ContextInfo info, Folder folder,
{bool optionsRemoved: false}) {
Map<String, YamlNode> options;
Map<String, Object> options;
try {
options = analysisOptionsProvider.getOptions(folder);
} catch (_) {
Expand All @@ -550,6 +550,24 @@ class ContextManagerImpl implements ContextManager {
return;
}

// In case options files are removed, revert to defaults.
if (optionsRemoved) {
// Start with defaults.
info.context.analysisOptions = new AnalysisOptionsImpl();

// Apply inherited options.
options = _getEmbeddedOptions(info.context);
if (options != null) {
configureContextOptions(info.context, options);
}
} else {
// Check for embedded options.
YamlMap embeddedOptions = _getEmbeddedOptions(info.context);
if (embeddedOptions != null) {
options = new Merger().merge(embeddedOptions, options);
}
}

// Notify options processors.
AnalysisEngine.instance.optionsPlugin.optionsProcessors
.forEach((OptionsProcessor p) {
Expand All @@ -562,34 +580,19 @@ class ContextManagerImpl implements ContextManager {
}
});

// In case options files are removed, revert to defaults.
if (optionsRemoved) {
// Start with defaults.
info.context.analysisOptions = new AnalysisOptionsImpl();
configureContextOptions(info.context, options);

// Apply inherited options.
YamlMap embeddedOptions = _getEmbeddedOptions(info.context);
if (embeddedOptions != null) {
configureContextOptions(info.context, embeddedOptions);
}
// Nothing more to do.
if (options == null) {
return;
}

// Check for embedded options.
YamlMap embeddedOptions = _getEmbeddedOptions(info.context);
if (embeddedOptions != null) {
options = new Merger().merge(embeddedOptions, options);
}

// Analysis options are processed 'in-line'.
var analyzer = options[AnalyzerOptions.analyzer];
if (analyzer is! Map) {
// No options for analyzer.
// Done.
return;
}

configureContextOptions(info.context, options);

// Set ignore patterns.
YamlList exclude = analyzer[AnalyzerOptions.exclude];
if (exclude != null) {
Expand Down
214 changes: 169 additions & 45 deletions pkg/analysis_server/test/context_manager_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,12 @@ import 'package:analyzer/src/generated/engine.dart';
import 'package:analyzer/src/generated/error.dart';
import 'package:analyzer/src/generated/source.dart';
import 'package:analyzer/src/generated/source_io.dart';
import 'package:analyzer/src/services/lint.dart';
import 'package:linter/src/plugin/linter_plugin.dart';
import 'package:linter/src/rules/avoid_as.dart';
import 'package:package_config/packages.dart';
import 'package:path/path.dart';
import 'package:plugin/plugin.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
import 'package:unittest/unittest.dart';

Expand Down Expand Up @@ -67,6 +71,34 @@ class AbstractContextManagerTest {

String projPath = '/my/proj';

AnalysisError missing_return =
new AnalysisError(new TestSource(), 0, 1, HintCode.MISSING_RETURN, [
['x']
]);

AnalysisError invalid_assignment_error =
new AnalysisError(new TestSource(), 0, 1, HintCode.INVALID_ASSIGNMENT, [
['x'],
['y']
]);

AnalysisError unused_local_variable = new AnalysisError(
new TestSource(), 0, 1, HintCode.UNUSED_LOCAL_VARIABLE, [
['x']
]);

List<ErrorFilter> get errorFilters =>
callbacks.currentContext.getConfigurationData(CONFIGURED_ERROR_FILTERS);

List<Linter> get lints => getLints(callbacks.currentContext);

AnalysisOptions get options => callbacks.currentContext.analysisOptions;

void deleteFile(List<String> pathComponents) {
String filePath = posix.joinAll(pathComponents);
resourceProvider.deleteFile(filePath);
}

String newFile(List<String> pathComponents, [String content = '']) {
String filePath = posix.joinAll(pathComponents);
resourceProvider.newFile(filePath, content);
Expand All @@ -84,6 +116,14 @@ class AbstractContextManagerTest {
}

void setUp() {
List<Plugin> plugins = <Plugin>[];
plugins.add(linterPlugin);

// Defer to the extension manager in AE for plugin registration.
AnalysisEngine.instance.userDefinedPlugins = plugins;
// Force registration.
AnalysisEngine.instance.taskManager;

resourceProvider = new MemoryResourceProvider();
packageMapProvider = new MockPackageMapProvider();
manager = new ContextManagerImpl(resourceProvider, providePackageResolver,
Expand All @@ -93,6 +133,99 @@ class AbstractContextManagerTest {
resourceProvider.newFolder(projPath);
}

test_analysis_options_file_delete() async {
// Setup .analysis_options
newFile(
[projPath, AnalysisEngine.ANALYSIS_OPTIONS_FILE],
r'''
embedder_libs:
"dart:foobar": "../sdk_ext/entry.dart"
analyzer:
language:
enableGenericMethods: true
errors:
unused_local_variable: false
linter:
rules:
- camel_case_types
''');

// Setup context.
manager.setRoots(<String>[projPath], <String>[], <String, String>{});
await pumpEventQueue();

// Verify options were set.
expect(errorFilters, hasLength(1));
expect(lints, hasLength(1));
expect(options.enableGenericMethods, isTrue);

// Remove options.
deleteFile([projPath, AnalysisEngine.ANALYSIS_OPTIONS_FILE]);
await pumpEventQueue();

// Verify defaults restored.
expect(errorFilters, isEmpty);
expect(lints, isEmpty);
expect(options.enableGenericMethods, isFalse);
}

test_analysis_options_file_delete_with_embedder() async {
// Setup _embedder.yaml.
String libPath = newFolder([projPath, LIB_NAME]);
newFile(
[libPath, '_embedder.yaml'],
r'''
analyzer:
strong-mode: true
errors:
missing_return: false
linter:
rules:
- avoid_as
''');

// Setup .packages file
newFile(
[projPath, '.packages'],
r'''
test_pack:lib/''');

// Setup .analysis_options
newFile(
[projPath, AnalysisEngine.ANALYSIS_OPTIONS_FILE],
r'''
analyzer:
language:
enableGenericMethods: true
errors:
unused_local_variable: false
linter:
rules:
- camel_case_types
''');

// Setup context.
manager.setRoots(<String>[projPath], <String>[], <String, String>{});
await pumpEventQueue();

// Verify options were set.
expect(options.enableGenericMethods, isTrue);
expect(options.strongMode, isTrue);
expect(errorFilters, hasLength(2));
expect(lints, hasLength(2));

// Remove options.
deleteFile([projPath, AnalysisEngine.ANALYSIS_OPTIONS_FILE]);
await pumpEventQueue();

// Verify defaults restored.
expect(options.enableGenericMethods, isFalse);
expect(lints, hasLength(1));
expect(lints.first, new isInstanceOf<AvoidAs>());
expect(errorFilters, hasLength(1));
expect(errorFilters.first(missing_return), isTrue);
}

test_analysis_options_parse_failure() async {
// Create files.
String libPath = newFolder([projPath, LIB_NAME]);
Expand Down Expand Up @@ -154,6 +287,11 @@ analyzer:
strong-mode: true
language:
enableSuperMixins: true
errors:
missing_return: false
linter:
rules:
- avoid_as
''');
// Setup .packages file
newFile(
Expand All @@ -172,6 +310,9 @@ analyzer:
enableGenericMethods: true
errors:
unused_local_variable: false
linter:
rules:
- camel_case_types
''');

// Setup context.
Expand All @@ -186,25 +327,31 @@ analyzer:
var context = contexts[0];

// Verify options.
// * from `_embedder.yaml`:
// * from `_embedder.yaml`:
expect(context.analysisOptions.strongMode, isTrue);
expect(context.analysisOptions.enableSuperMixins, isTrue);
// * from `.analysis_options`:
// * from `.analysis_options`:
expect(context.analysisOptions.enableGenericMethods, isTrue);
// verify tests are excluded
// * verify tests are excluded
expect(callbacks.currentContextFilePaths[projPath].keys,
['/my/proj/sdk_ext/entry.dart']);

// Verify filter setup.
List<ErrorFilter> filters =
callbacks.currentContext.getConfigurationData(CONFIGURED_ERROR_FILTERS);
expect(filters, hasLength(1));
expect(errorFilters, hasLength(2));

// * (embedder.)
expect(errorFilters.any((f) => f(missing_return)), isTrue);

// * (options.)
expect(errorFilters.any((f) => f(unused_local_variable)), isTrue);

// Verify lints.
var lintNames = lints.map((lint) => lint.name);

expect(
filters.first(new AnalysisError(
new TestSource(), 0, 1, HintCode.UNUSED_LOCAL_VARIABLE, [
['x']
])),
isTrue);
lintNames,
unorderedEquals(
['avoid_as' /* embedder */, 'camel_case_types' /* options */]));

// Sanity check embedder libs.
var source = context.sourceFactory.forUri('dart:foobar');
Expand Down Expand Up @@ -268,16 +415,9 @@ analyzer:
manager.setRoots(<String>[projPath], <String>[], <String, String>{});

// Verify filter setup.
List<ErrorFilter> filters =
callbacks.currentContext.getConfigurationData(CONFIGURED_ERROR_FILTERS);
expect(filters, isNotNull);
expect(filters, hasLength(1));
expect(
filters.first(new AnalysisError(
new TestSource(), 0, 1, HintCode.UNUSED_LOCAL_VARIABLE, [
['x']
])),
isTrue);
expect(errorFilters, isNotNull);
expect(errorFilters, hasLength(1));
expect(errorFilters.first(unused_local_variable), isTrue);
}

test_error_filter_analysis_option_multiple_filters() async {
Expand All @@ -294,24 +434,12 @@ analyzer:
manager.setRoots(<String>[projPath], <String>[], <String, String>{});

// Verify filter setup.
List<ErrorFilter> filters =
callbacks.currentContext.getConfigurationData(CONFIGURED_ERROR_FILTERS);
expect(filters, isNotNull);
expect(filters, hasLength(2));

var unused_error = new AnalysisError(
new TestSource(), 0, 1, HintCode.UNUSED_LOCAL_VARIABLE, [
['x']
]);
expect(errorFilters, isNotNull);
expect(errorFilters, hasLength(2));

var invalid_assignment_error =
new AnalysisError(new TestSource(), 0, 1, HintCode.INVALID_ASSIGNMENT, [
['x'],
['y']
]);

expect(filters.any((filter) => filter(unused_error)), isTrue);
expect(filters.any((filter) => filter(invalid_assignment_error)), isTrue);
expect(errorFilters.any((filter) => filter(unused_local_variable)), isTrue);
expect(
errorFilters.any((filter) => filter(invalid_assignment_error)), isTrue);
}

test_error_filter_analysis_option_synonyms() async {
Expand All @@ -328,10 +456,8 @@ analyzer:
manager.setRoots(<String>[projPath], <String>[], <String, String>{});

// Verify filter setup.
List<ErrorFilter> filters =
callbacks.currentContext.getConfigurationData(CONFIGURED_ERROR_FILTERS);
expect(filters, isNotNull);
expect(filters, hasLength(2));
expect(errorFilters, isNotNull);
expect(errorFilters, hasLength(2));
}

test_error_filter_analysis_option_unpsecified() async {
Expand All @@ -347,9 +473,7 @@ analyzer:
manager.setRoots(<String>[projPath], <String>[], <String, String>{});

// Verify filter setup.
List<ErrorFilter> filters =
callbacks.currentContext.getConfigurationData(CONFIGURED_ERROR_FILTERS);
expect(filters, isEmpty);
expect(errorFilters, isEmpty);
}

test_ignoreFilesInPackagesFolder() {
Expand Down
Loading

0 comments on commit 1e4cea3

Please sign in to comment.