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

add lint depend_on_referenced_packages #2659

Merged
merged 9 commits into from
May 19, 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 example/all.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ linter:
- constant_identifier_names
- control_flow_in_finally
- curly_braces_in_flow_control_structures
- depend_on_referenced_packages
- deprecated_consistency
- diagnostic_describe_all_properties
- directives_ordering
Expand Down
4 changes: 3 additions & 1 deletion lib/src/analyzer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@ export 'package:analyzer/src/lint/linter.dart'
NodeLintRule;
export 'package:analyzer/src/lint/project.dart'
show DartProject, ProjectVisitor;
export 'package:analyzer/src/lint/pub.dart' show PubspecVisitor, PSEntry;
export 'package:analyzer/src/lint/pub.dart'
show PubspecVisitor, PSEntry, Pubspec;
export 'package:analyzer/src/lint/util.dart' show Spelunker;
export 'package:analyzer/src/services/lint.dart' show lintRegistry;
export 'package:analyzer/src/workspace/pub.dart' show PubWorkspacePackage;

const loggedAnalyzerErrorExitCode = 63;

Expand Down
14 changes: 14 additions & 0 deletions lib/src/ast.dart
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,20 @@ bool isInLibDir(CompilationUnit node, WorkspacePackage? package) {
return path.isWithin(libDir, cuPath);
}

/// Return `true` if this compilation unit [node] is declared within a public
/// directory in the given [package]'s directory tree. Public dirs are the
/// `lib` and `bin` dirs.
//
// TODO: move into WorkspacePackage
bool isInPublicDir(CompilationUnit node, WorkspacePackage? package) {
jakemac53 marked this conversation as resolved.
Show resolved Hide resolved
if (package == null) return false;
var cuPath = node.declaredElement?.library.source.fullName;
if (cuPath == null) return false;
var libDir = path.join(package.root, 'lib');
var binDir = path.join(package.root, 'bin');
return path.isWithin(libDir, cuPath) || path.isWithin(binDir, cuPath);
}

/// Returns `true` if the keyword associated with the given [token] matches
/// [keyword].
bool isKeyword(Token token, Keyword keyword) =>
Expand Down
2 changes: 2 additions & 0 deletions lib/src/rules.dart
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import 'rules/comment_references.dart';
import 'rules/constant_identifier_names.dart';
import 'rules/control_flow_in_finally.dart';
import 'rules/curly_braces_in_flow_control_structures.dart';
import 'rules/depend_on_referenced_packages.dart';
import 'rules/deprecated_consistency.dart';
import 'rules/diagnostic_describe_all_properties.dart';
import 'rules/directives_ordering.dart';
Expand Down Expand Up @@ -257,6 +258,7 @@ void registerLintRules({bool inTestMode = false}) {
..register(ConstantIdentifierNames())
..register(ControlFlowInFinally())
..register(CurlyBracesInFlowControlStructures())
..register(DependOnReferencedPackages())
..register(DeprecatedConsistency())
..register(DiagnosticsDescribeAllProperties())
..register(DirectivesOrdering())
Expand Down
109 changes: 109 additions & 0 deletions lib/src/rules/depend_on_referenced_packages.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
// Copyright (c) 2021, 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 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';

import '../analyzer.dart';
import '../ast.dart';

const _desc = r'Depend on referenced packages.';

const _details = r'''

**DO** Depend on referenced packages.

When importing a package, add a dependency on it to your pubspec.

Depending explicitly on packages that you reference ensures they will always
exist and allows you to put a dependency constraint on them to guard you
against breaking changes.

Whether this should be a regular dependency or dev_dependency depends on if it
is referenced from a public file (one under either `lib` or `bin`), or some
other private file.

**BAD:**
```dart
import 'package:a/a.dart';
```

```yaml
dependencies:
```

**GOOD:**
```dart
import 'package:a/a.dart';
```

```yaml
dependencies:
a: ^1.0.0
```

''';

class DependOnReferencedPackages extends LintRule implements NodeLintRule {
DependOnReferencedPackages()
: super(
name: 'depend_on_referenced_packages',
description: _desc,
details: _details,
group: Group.pub);

@override
void registerNodeProcessors(
NodeLintRegistry registry, LinterContext context) {
// Only lint if we have a pubspec.
var package = context.package;
if (package is! PubWorkspacePackage) return;
var pubspec = package.pubspec;
if (pubspec == null) return;

var dependencies = pubspec.dependencies;
var devDependencies = pubspec.devDependencies;
var availableDeps = [
if (dependencies != null)
for (var dep in dependencies)
if (dep.name?.text != null) dep.name!.text!,
if (devDependencies != null &&
!isInPublicDir(context.currentUnit.unit, context.package))
for (var dep in devDependencies)
if (dep.name?.text != null) dep.name!.text!,
];

var visitor = _Visitor(this, availableDeps);
registry.addImportDirective(this, visitor);
registry.addExportDirective(this, visitor);
}
}

class _Visitor extends SimpleAstVisitor {
final DependOnReferencedPackages rule;
final List<String> availableDeps;

_Visitor(this.rule, this.availableDeps);

void _checkDirective(UriBasedDirective node) {
// Is it a package: uri?
var uriContent = node.uriContent;
if (uriContent == null) return;
if (!uriContent.startsWith('package:')) return;

// The package name is the first segment of the uri, find the first slash.
var firstSlash = uriContent.indexOf('/');
if (firstSlash == -1) return;

var packageName = uriContent.substring(8, firstSlash);
if (availableDeps.contains(packageName)) return;
rule.reportLint(node.uri);
}

@override
void visitImportDirective(ImportDirective node) => _checkDirective(node);

@override
void visitExportDirective(ExportDirective node) => _checkDirective(node);
}
93 changes: 93 additions & 0 deletions test/integration/depend_on_referenced_packages.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Copyright (c) 2021, 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:io';

import 'package:analyzer/src/lint/io.dart';
import 'package:linter/src/cli.dart' as cli;
import 'package:test/test.dart';

import '../mocks.dart';
import '../test_constants.dart';

void main() {
group('Depend on referenced packages', () {
var currentOut = outSink;
var collectingOut = CollectingSink();
setUp(() {
exitCode = 0;
outSink = collectingOut;
});
tearDown(() {
collectingOut.buffer.clear();
outSink = currentOut;
exitCode = 0;
});

test('lints files under bin', () async {
var packagesFilePath = File('.packages').absolute.path;
await cli.run([
'--packages',
packagesFilePath,
'$integrationTestDir/depend_on_referenced_packages/bin',
'--rules=depend_on_referenced_packages'
]);
expect(
collectingOut.trim(),
stringContainsInOrder([
"Depend on referenced packages.",
"import 'package:test/test.dart'; // LINT",
"Depend on referenced packages.",
"import 'package:matcher/matcher.dart'; // LINT",
"Depend on referenced packages.",
"export 'package:test/test.dart'; // LINT",
"Depend on referenced packages.",
"export 'package:matcher/matcher.dart'; // LINT",
]));
expect(exitCode, 1);
});

test('lints files under lib', () async {
var packagesFilePath = File('.packages').absolute.path;
await cli.run([
'--packages',
packagesFilePath,
'$integrationTestDir/depend_on_referenced_packages/lib',
'--rules=depend_on_referenced_packages'
]);
expect(
collectingOut.trim(),
stringContainsInOrder([
"Depend on referenced packages.",
"import 'package:test/test.dart'; // LINT",
"Depend on referenced packages.",
"import 'package:matcher/matcher.dart'; // LINT",
"Depend on referenced packages.",
"export 'package:test/test.dart'; // LINT",
"Depend on referenced packages.",
"export 'package:matcher/matcher.dart'; // LINT",
]));
expect(exitCode, 1);
});

test('lints files under test', () async {
var packagesFilePath = File('.packages').absolute.path;
await cli.run([
'--packages',
packagesFilePath,
'$integrationTestDir/depend_on_referenced_packages/test',
'--rules=depend_on_referenced_packages'
]);
expect(
collectingOut.trim(),
stringContainsInOrder([
"Depend on referenced packages.",
"import 'package:matcher/matcher.dart'; // LINT",
"Depend on referenced packages.",
"export 'package:matcher/matcher.dart'; // LINT",
]));
expect(exitCode, 1);
});
});
}
3 changes: 3 additions & 0 deletions test/integration_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import 'integration/avoid_web_libraries_in_flutter.dart'
as avoid_web_libraries_in_flutter;
import 'integration/cancel_subscriptions.dart' as cancel_subscriptions;
import 'integration/close_sinks.dart' as close_sinks;
import 'integration/depend_on_referenced_packages.dart'
as depend_on_referenced_packages;
import 'integration/directives_ordering.dart' as directives_ordering;
import 'integration/exhaustive_cases.dart' as exhaustive_cases;
import 'integration/file_names.dart' as file_names;
Expand Down Expand Up @@ -185,6 +187,7 @@ void ruleTests() {
overridden_fields.main();
close_sinks.main();
cancel_subscriptions.main();
depend_on_referenced_packages.main();
directives_ordering.main();
file_names.main();
flutter_style_todos.main();
Expand Down
11 changes: 11 additions & 0 deletions test_data/integration/depend_on_referenced_packages/bin/main.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) 2021, 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 'package:meta/meta.dart'; // OK
import 'package:test/test.dart'; // LINT
import 'package:matcher/matcher.dart'; // LINT

export 'package:meta/meta.dart'; // OK
export 'package:test/test.dart'; // LINT
export 'package:matcher/matcher.dart'; // LINT
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) 2021, 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 'package:meta/meta.dart'; // OK
import 'package:test/test.dart'; // LINT
import 'package:matcher/matcher.dart'; // LINT

export 'package:meta/meta.dart'; // OK
export 'package:test/test.dart'; // LINT
export 'package:matcher/matcher.dart'; // LINT
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
name: sample_project
dependencies:
meta: any
dev_dependencies:
test: any
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) 2021, 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 'package:meta/meta.dart'; // OK
import 'package:test/test.dart'; // OK
import 'package:matcher/matcher.dart'; // LINT

export 'package:meta/meta.dart'; // OK
export 'package:test/test.dart'; // OK
export 'package:matcher/matcher.dart'; // LINT