-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Move the linter core to the analyzer package
R=scheglov@google.com Review URL: https://codereview.chromium.org/2553203002 .
- Loading branch information
1 parent
ea11c82
commit 6d9d5f5
Showing
17 changed files
with
1,796 additions
and
8 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,262 @@ | ||
| // Copyright (c) 2015, 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:collection'; | ||
| import 'dart:io' as io; | ||
|
|
||
| import 'package:analyzer/dart/element/element.dart'; | ||
| import 'package:analyzer/file_system/file_system.dart' | ||
| show File, Folder, ResourceProvider, ResourceUriResolver; | ||
| import 'package:analyzer/file_system/physical_file_system.dart'; | ||
| import 'package:analyzer/source/package_map_resolver.dart'; | ||
| import 'package:analyzer/src/context/builder.dart'; | ||
| import 'package:analyzer/src/dart/sdk/sdk.dart'; | ||
| import 'package:analyzer/src/generated/engine.dart'; | ||
| import 'package:analyzer/src/generated/sdk.dart'; | ||
| import 'package:analyzer/src/generated/source.dart'; | ||
| import 'package:analyzer/src/generated/source_io.dart'; | ||
| import 'package:analyzer/src/lint/io.dart'; | ||
| import 'package:analyzer/src/lint/linter.dart'; | ||
| import 'package:analyzer/src/lint/project.dart'; | ||
| import 'package:analyzer/src/lint/registry.dart'; | ||
| import 'package:analyzer/src/services/lint.dart'; | ||
| import 'package:cli_util/cli_util.dart' as cli_util; | ||
| import 'package:package_config/packages.dart' show Packages; | ||
| import 'package:package_config/packages_file.dart' as pkgfile show parse; | ||
| import 'package:package_config/src/packages_impl.dart' show MapPackages; | ||
| import 'package:path/path.dart' as p; | ||
| import 'package:plugin/manager.dart'; | ||
| import 'package:plugin/plugin.dart'; | ||
|
|
||
| Source createSource(Uri sourceUri) { | ||
| return PhysicalResourceProvider.INSTANCE | ||
| .getFile(sourceUri.toFilePath()) | ||
| .createSource(sourceUri); | ||
| } | ||
|
|
||
| /// Print the given message and exit with the given [exitCode] | ||
| void printAndFail(String message, {int exitCode: 15}) { | ||
| print(message); | ||
| io.exit(exitCode); | ||
| } | ||
|
|
||
| AnalysisOptions _buildAnalyzerOptions(DriverOptions options) { | ||
| AnalysisOptionsImpl analysisOptions = new AnalysisOptionsImpl(); | ||
| analysisOptions.strongMode = options.strongMode; | ||
| analysisOptions.hint = false; | ||
| analysisOptions.lint = options.enableLints; | ||
| analysisOptions.generateSdkErrors = options.showSdkWarnings; | ||
| analysisOptions.enableTiming = options.enableTiming; | ||
| return analysisOptions; | ||
| } | ||
|
|
||
| class AnalysisDriver { | ||
| /// The sources which have been analyzed so far. This is used to avoid | ||
| /// analyzing a source more than once, and to compute the total number of | ||
| /// sources analyzed for statistics. | ||
| Set<Source> _sourcesAnalyzed = new HashSet<Source>(); | ||
|
|
||
| final LinterOptions options; | ||
|
|
||
| AnalysisDriver(this.options) { | ||
| _processPlugins(); | ||
| } | ||
|
|
||
| /// Return the number of sources that have been analyzed so far. | ||
| int get numSourcesAnalyzed => _sourcesAnalyzed.length; | ||
|
|
||
| List<UriResolver> get resolvers { | ||
| // TODO(brianwilkerson) Use the context builder to compute all of the resolvers. | ||
| ResourceProvider resourceProvider = PhysicalResourceProvider.INSTANCE; | ||
| ContextBuilder builder = new ContextBuilder(resourceProvider, null, null); | ||
|
|
||
| DartSdk sdk = options.mockSdk ?? | ||
| new FolderBasedDartSdk( | ||
| resourceProvider, resourceProvider.getFolder(sdkDir)); | ||
|
|
||
| List<UriResolver> resolvers = [new DartUriResolver(sdk)]; | ||
|
|
||
| if (options.packageRootPath != null) { | ||
| // TODO(brianwilkerson) After 0.30.0 is published, clean up the following. | ||
| try { | ||
| // Try to use the post 0.30.0 API. | ||
| (builder as dynamic).builderOptions.defaultPackagesDirectoryPath = | ||
| options.packageRootPath; | ||
| } catch (_) { | ||
| // If that fails, fall back to the pre 0.30.0 API. | ||
| (builder as dynamic).defaultPackagesDirectoryPath = | ||
| options.packageRootPath; | ||
| } | ||
| Map<String, List<Folder>> packageMap = | ||
| builder.convertPackagesToMap(builder.createPackageMap(null)); | ||
| resolvers.add(new PackageMapUriResolver(resourceProvider, packageMap)); | ||
| } | ||
|
|
||
| // File URI resolver must come last so that files inside "/lib" are | ||
| // are analyzed via "package:" URI's. | ||
| resolvers.add(new ResourceUriResolver(resourceProvider)); | ||
| return resolvers; | ||
| } | ||
|
|
||
| String get sdkDir { | ||
| if (options.dartSdkPath != null) { | ||
| return options.dartSdkPath; | ||
| } | ||
| // In case no SDK has been specified, fall back to inferring it | ||
| // TODO: pass args to cli_util | ||
| return cli_util.getSdkDir().path; | ||
| } | ||
|
|
||
| List<AnalysisErrorInfo> analyze(Iterable<io.File> files) { | ||
| AnalysisContext context = AnalysisEngine.instance.createAnalysisContext(); | ||
| context.analysisOptions = _buildAnalyzerOptions(options); | ||
| registerLinters(context); | ||
|
|
||
| Packages packages = _getPackageConfig(); | ||
|
|
||
| context.sourceFactory = new SourceFactory(resolvers, packages); | ||
| AnalysisEngine.instance.logger = new StdLogger(); | ||
|
|
||
| List<Source> sources = []; | ||
| ChangeSet changeSet = new ChangeSet(); | ||
| for (io.File file in files) { | ||
| File sourceFile = PhysicalResourceProvider.INSTANCE | ||
| .getFile(p.normalize(file.absolute.path)); | ||
| Source source = sourceFile.createSource(); | ||
| Uri uri = context.sourceFactory.restoreUri(source); | ||
| if (uri != null) { | ||
| // Ensure that we analyze the file using its canonical URI (e.g. if | ||
| // it's in "/lib", analyze it using a "package:" URI). | ||
| source = sourceFile.createSource(uri); | ||
| } | ||
| sources.add(source); | ||
| changeSet.addedSource(source); | ||
| } | ||
| context.applyChanges(changeSet); | ||
|
|
||
| // Temporary location | ||
| var project = new DartProject(context, sources); | ||
| // This will get pushed into the generator (or somewhere comparable) when | ||
| // we have a proper plugin. | ||
| Registry.ruleRegistry.forEach((lint) { | ||
| if (lint is ProjectVisitor) { | ||
| (lint as ProjectVisitor).visit(project); | ||
| } | ||
| }); | ||
|
|
||
| List<AnalysisErrorInfo> errors = []; | ||
|
|
||
| for (Source source in sources) { | ||
| context.computeErrors(source); | ||
| errors.add(context.getErrors(source)); | ||
| _sourcesAnalyzed.add(source); | ||
| } | ||
|
|
||
| if (options.visitTransitiveClosure) { | ||
| // In the process of computing errors for all the sources in [sources], | ||
| // the analyzer has visited the transitive closure of all libraries | ||
| // referenced by those sources. So now we simply need to visit all | ||
| // library sources known to the analysis context, and all parts they | ||
| // refer to. | ||
| for (Source librarySource in context.librarySources) { | ||
| for (Source source in _getAllUnitSources(context, librarySource)) { | ||
| if (!_sourcesAnalyzed.contains(source)) { | ||
| context.computeErrors(source); | ||
| errors.add(context.getErrors(source)); | ||
| _sourcesAnalyzed.add(source); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return errors; | ||
| } | ||
|
|
||
| void registerLinters(AnalysisContext context) { | ||
| if (options.enableLints) { | ||
| setLints(context, options.enabledLints?.toList(growable: false)); | ||
| } | ||
| } | ||
|
|
||
| /// Yield the sources for all the compilation units constituting | ||
| /// [librarySource] (including the defining compilation unit). | ||
| Iterable<Source> _getAllUnitSources( | ||
| AnalysisContext context, Source librarySource) { | ||
| List<Source> result = <Source>[librarySource]; | ||
| result.addAll(context | ||
| .getLibraryElement(librarySource) | ||
| .parts | ||
| .map((CompilationUnitElement e) => e.source)); | ||
| return result; | ||
| } | ||
|
|
||
| Packages _getPackageConfig() { | ||
| if (options.packageConfigPath != null) { | ||
| String packageConfigPath = options.packageConfigPath; | ||
| Uri fileUri = new Uri.file(packageConfigPath); | ||
| try { | ||
| io.File configFile = new io.File.fromUri(fileUri).absolute; | ||
| List<int> bytes = configFile.readAsBytesSync(); | ||
| Map<String, Uri> map = pkgfile.parse(bytes, configFile.uri); | ||
| return new MapPackages(map); | ||
| } catch (e) { | ||
| printAndFail( | ||
| 'Unable to read package config data from $packageConfigPath: $e'); | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| void _processPlugins() { | ||
| List<Plugin> plugins = <Plugin>[]; | ||
| plugins.addAll(AnalysisEngine.instance.requiredPlugins); | ||
| plugins.add(AnalysisEngine.instance.commandLinePlugin); | ||
| plugins.add(AnalysisEngine.instance.optionsPlugin); | ||
| ExtensionManager manager = new ExtensionManager(); | ||
| manager.processPlugins(plugins); | ||
| } | ||
| } | ||
|
|
||
| class DriverOptions { | ||
| /// The maximum number of sources for which AST structures should be kept | ||
| /// in the cache. The default is 512. | ||
| int cacheSize = 512; | ||
|
|
||
| /// The path to the dart SDK. | ||
| String dartSdkPath; | ||
|
|
||
| /// Whether to show lint warnings. | ||
| bool enableLints = true; | ||
|
|
||
| /// Whether to gather timing data during analysis. | ||
| bool enableTiming = false; | ||
|
|
||
| /// The path to a `.packages` configuration file | ||
| String packageConfigPath; | ||
|
|
||
| /// The path to the package root. | ||
| String packageRootPath; | ||
|
|
||
| /// Whether to show SDK warnings. | ||
| bool showSdkWarnings = false; | ||
|
|
||
| /// Whether to use Dart's Strong Mode analyzer. | ||
| bool strongMode = true; | ||
|
|
||
| /// The mock SDK (to speed up testing) or `null` to use the actual SDK. | ||
| DartSdk mockSdk; | ||
|
|
||
| /// Whether to show lints for the transitive closure of imported and exported | ||
| /// libraries. | ||
| bool visitTransitiveClosure = false; | ||
| } | ||
|
|
||
| /// Prints logging information comments to the [outSink] and error messages to | ||
| /// [errorSink]. | ||
| class StdLogger extends Logger { | ||
| @override | ||
| void logError(String message, [exception]) => errorSink.writeln(message); | ||
| @override | ||
| void logInformation(String message, [exception]) => outSink.writeln(message); | ||
| } |
Oops, something went wrong.
6d9d5f5There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no issue link or similar - what was the reasoning behind this change @bwilkerson ?
It seems like it makes contributing to the linter more difficult, there are now issues in the linter repo which can't be closed without editing these files.
6d9d5f5There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code that actually runs lints (the code moved by the CL) is strongly coupled to the implementation of analysis. Having it in a separate package that was pulled in to the SDK via DEPS made it difficult for us to change this code in tandem with changes in analyzer. Moving the code improved our velocity.
I'm not sure which issues you're referring to, so I can't respond very specifically, but it's quite possible that those issues should be moved to the sdk repo's issue tracker. I'm not aware of any issues that would require changes to both the lint running code and to specific lint rules. I'm happy to discuss specific issues if I'm mistaken.
As a counterpoint, I'll note that contributions to the linter package have not decreased as a result of moving this code.
There is a strong argument to be made for moving the whole linter package to the sdk repo because the lint rules are also strongly coupled with the analyzer APIs, and keeping the version of linter that is pulled into the SDK consistent is also a pain. But there's concern that moving it would have a negative impact on the rate of contributions.
6d9d5f5There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we considered the structure of the Linter is wrong?
I still can't wrap my head around a linter package that requires all rules to live in a single centralized package (in this case, that is going to ship with the SDK).
Couldn't it just be an analysis plugin now like Angular?
6d9d5f5There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it could be, and I've considered the possibility. (Note that this has only become a real possibility in the last month or so.) I think there are a lot of advantages to such a scheme. The biggest disadvantage I've thought of so far is that users would explicitly need to opt-in to having the linter running (in addition to having to opt-in to specific lints). We'd also need to assess the performance impact of making such a change. But the idea is definitely appealing.
6d9d5f5There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting conversation!
A few quick thoughts:
_linter#684) was an unintended side effect and being remedied now.6d9d5f5There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That all makes sense. Thanks!