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

Linter for drift #3281

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Prev Previous commit
Next Next commit
temp
dickermoshe committed Dec 12, 2024
commit 178e0fca711b01ca5aea93974f74b9d540a15055
17 changes: 3 additions & 14 deletions drift_dev/lib/drift_dev.dart
Original file line number Diff line number Diff line change
@@ -1,18 +1,7 @@
import 'package:custom_lint_builder/custom_lint_builder.dart';
import 'package:drift_dev/src/lints/drift_errors.dart';
import 'package:drift_dev/src/lints/unawaited_futures_in_transaction.dart';

import 'src/lints/column_builder_on_table.dart';
import 'package:drift_dev/src/lints/custom_lint_plugin.dart';

/// This function is automaticly recognized by custom_lint to include this drift_dev package as a linter
PluginBase createPlugin() {
Copy link
Owner

Choose a reason for hiding this comment

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

Not a huge fan of this, is there a way to tell custom_lint to use a different import for a package providing linters (my preferred library would be lib/integrations/custom_lint.dart).

But if that's not possible so be it, I can file an issue on custom_lint and we can leave this as-is until that gets resolved.

return _DriftLinter();
}

class _DriftLinter extends PluginBase {
@override
List<LintRule> getLintRules(CustomLintConfigs configs) => [
ColumnBuilderOnTable(),
UnawaitedFuturesInTransaction(),
DriftBuildErrors()
];
return DriftLinter();
}
26 changes: 18 additions & 8 deletions drift_dev/lib/src/analysis/driver/error.dart
Original file line number Diff line number Diff line change
@@ -4,39 +4,49 @@ import 'package:source_gen/source_gen.dart';
import 'package:source_span/source_span.dart';
import 'package:sqlparser/sqlparser.dart' as sql;

enum DriftAnalysisErrorLevel { warning, error }

class DriftAnalysisError {
final SourceSpan? span;
final String message;
final DriftAnalysisErrorLevel level;

DriftAnalysisError(this.span, this.message);
DriftAnalysisError(this.span, this.message,
{this.level = DriftAnalysisErrorLevel.error});

factory DriftAnalysisError.forDartElement(
dart.Element element, String message) {
dart.Element element, String message,
{DriftAnalysisErrorLevel level = DriftAnalysisErrorLevel.error}) {
return DriftAnalysisError(
spanForElement(element),
message,
level: level,
);
}

factory DriftAnalysisError.inDartAst(
dart.Element element, dart.SyntacticEntity entity, String message) {
return DriftAnalysisError(dartAstSpan(element, entity), message);
dart.Element element, dart.SyntacticEntity entity, String message,
{DriftAnalysisErrorLevel level = DriftAnalysisErrorLevel.error}) {
return DriftAnalysisError(dartAstSpan(element, entity), message,
level: level);
}

factory DriftAnalysisError.inDriftFile(
sql.SyntacticEntity sql, String message) {
return DriftAnalysisError(sql.span, message);
sql.SyntacticEntity sql, String message,
{DriftAnalysisErrorLevel level = DriftAnalysisErrorLevel.error}) {
return DriftAnalysisError(sql.span, message, level: level);
}

factory DriftAnalysisError.fromSqlError(sql.AnalysisError error) {
factory DriftAnalysisError.fromSqlError(sql.AnalysisError error,
{DriftAnalysisErrorLevel level = DriftAnalysisErrorLevel.error}) {
var message = error.message ?? '';
if (error.type == sql.AnalysisErrorType.notSupportedInDesiredVersion) {
message =
'$message\nNote: You can change the assumed sqlite version with build '
'options. See https://drift.simonbinder.eu/options/#assumed-sql-environment for details!';
}

return DriftAnalysisError(error.span, message);
return DriftAnalysisError(error.span, message, level: level);
}

@override
8 changes: 8 additions & 0 deletions drift_dev/lib/src/analysis/resolver/dart/table.dart
Original file line number Diff line number Diff line change
@@ -42,6 +42,14 @@ class DartTableResolver extends LocalElementResolver<DiscoveredDartTable> {
} else {
for (final constraint in column.column.constraints) {
if (constraint is ForeignKeyReference) {
if (column.column.sqlType.builtin !=
constraint.otherColumn.sqlType.builtin ||
column.column.typeConverter?.dartType !=
constraint.otherColumn.typeConverter?.dartType) {
reportError(DriftAnalysisError.forDartElement(column.element,
"This column references a column whose type doesn't match this one. The generated managers will ignore this relation",
level: DriftAnalysisErrorLevel.warning));
}
references.add(constraint.otherColumn.owner);
}
}
46 changes: 0 additions & 46 deletions drift_dev/lib/src/lints/column_builder_on_table.dart

This file was deleted.

14 changes: 14 additions & 0 deletions drift_dev/lib/src/lints/custom_lint_plugin.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import 'package:custom_lint_builder/custom_lint_builder.dart';
import 'package:drift_dev/src/lints/offset_without_limit_lint.dart';
import 'package:drift_dev/src/lints/unawaited_futures_in_transaction_lint.dart';
import 'package:meta/meta.dart';

@internal
class DriftLinter extends PluginBase {
@override
List<LintRule> getLintRules(CustomLintConfigs configs) => [
unawaitedFuturesInMigration,
unawaitedFuturesInTransaction,
OffsetWithoutLimit(),
];
}
Original file line number Diff line number Diff line change
@@ -20,9 +20,8 @@ class DriftBuildErrors extends DartLintRule {
DriftBuildErrors() : super(code: _code);

static const _code = LintCode(
name: 'unawaited_futures_in_transaction',
problemMessage:
'All futures in a transaction should be awaited to ensure that all operations are completed before the transaction is closed.',
name: 'drift_build_errors',
problemMessage: '{0}',
errorSeverity: ErrorSeverity.ERROR,
);

@@ -34,11 +33,15 @@ class DriftBuildErrors extends DartLintRule {
final driver = DriftAnalysisDriver(backend, const DriftOptions.defaults());

final file = await driver.fullyAnalyze(unit.uri);
print(
'test? - ${unit.uri} - ${file.allErrors.length} - ${file.analysis.length}');
for (final error in file.allErrors) {
if (error.span case final span?) {
reporter.reportErrorForSpan(_code, span);
// ignore: deprecated_member_use
reporter.reportErrorForSpan(
error.level == DriftAnalysisErrorLevel.warning
? _codeAsWarning
: _code,
span,
[error.message.trim()]);
}
}
}
51 changes: 51 additions & 0 deletions drift_dev/lib/src/lints/offset_without_limit_lint.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/error/error.dart' hide LintCode;
import 'package:analyzer/error/listener.dart';
import 'package:custom_lint_builder/custom_lint_builder.dart';

final managerTypeChecker =
TypeChecker.fromName('BaseTableManager', packageName: 'drift');

class OffsetWithoutLimit extends DartLintRule {
OffsetWithoutLimit() : super(code: _code);

static const _code = LintCode(
name: 'offset_without_limit',
problemMessage: 'Using offset without a limit doesnt have any effect.',
errorSeverity: ErrorSeverity.ERROR,
);

@override
void run(CustomLintResolver resolver, ErrorReporter reporter,
CustomLintContext context) async {
context.registry.addMethodInvocation(
(node) {
if (node.argumentList.arguments.isEmpty) return;
final func = _typeCheck<SimpleIdentifier>(node.function);

if (func?.name == "get" || func?.name == "watch") {
final target = _typeCheck<PrefixedIdentifier>(node.target);
final managerGetter =
_typeCheck<PropertyAccessorElement>(target?.staticElement);
if (managerGetter != null) {
if (managerTypeChecker.isSuperTypeOf(managerGetter.returnType)) {
final namedArgs =
node.argumentList.arguments.whereType<NamedExpression>();
if (namedArgs
.every((element) => element.name.label.name != "limit") &&
namedArgs
.any((element) => element.name.label.name == "offset")) {
reporter.atNode(node, _code);
}
}
}
}
},
);
}
}

T? _typeCheck<T>(i) {
return i is T ? i : null;
}
70 changes: 0 additions & 70 deletions drift_dev/lib/src/lints/unawaited_futures_in_transaction.dart

This file was deleted.

196 changes: 196 additions & 0 deletions drift_dev/lib/src/lints/unawaited_futures_in_transaction_lint.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/error/error.dart' hide LintCode;
import 'package:analyzer/error/listener.dart';
import 'package:custom_lint_builder/custom_lint_builder.dart';

/// A lint rule that reports unawaited futures if an additional check returns true.
class _GenericUnawaitedFutureRule extends DartLintRule {
_GenericUnawaitedFutureRule(
{required super.code, required this.additionalCheck});

/// An unwaited future will only be reported if the [additionalCheck] returns true.
final bool Function(AstNode) additionalCheck;

@override
void run(CustomLintResolver resolver, ErrorReporter reporter,
CustomLintContext context) {
context.registry.addExpressionStatement((node) {
node.accept(
_Visitor(this, reporter, code, additionalCheck: additionalCheck));
});
context.registry.addCascadeExpression((node) {
node.accept(
_Visitor(this, reporter, code, additionalCheck: additionalCheck));
});
context.registry.addInterpolationExpression((node) {
node.accept(
_Visitor(this, reporter, code, additionalCheck: additionalCheck));
});
}
}

final _databaseConnectionUserChecker =
TypeChecker.fromName('DatabaseConnectionUser', packageName: 'drift');
final _migrationStrategyChecker =
TypeChecker.fromName('MigrationStrategy', packageName: 'drift');

/// A lint which reports unawaited futures in a transaction.
final unawaitedFuturesInTransaction = _GenericUnawaitedFutureRule(
code: LintCode(
name: 'unawaited_futures_in_transaction',
problemMessage:
'All futures in a transaction should be awaited to ensure that all operations are completed before the transaction is closed.',
errorSeverity: ErrorSeverity.ERROR,
),
additionalCheck: (node) {
return node.thisOrAncestorMatching(
(method) {
if (method is! MethodInvocation) return false;
final methodElement = method.methodName.staticElement;
if (methodElement is! MethodElement ||
methodElement.name != 'transaction') {
return false;
}
// ignore: deprecated_member_use
final enclosingElement = methodElement.enclosingElement;
if (enclosingElement is! ClassElement ||
!_databaseConnectionUserChecker.isExactly(enclosingElement)) {
return false;
}
return true;
},
) !=
null;
});

/// A lint which reports unawaited futures in a migration.
final unawaitedFuturesInMigration = _GenericUnawaitedFutureRule(
code: LintCode(
name: 'unawaited_futures_in_migration',
problemMessage:
'All futures in a migrations should be awaited to ensure that all operations are completed before the other opperations are performed.',
errorSeverity: ErrorSeverity.ERROR,
),
additionalCheck: (node) {
return node.thisOrAncestorMatching((node) =>
(node is InstanceCreationExpression &&
node.staticType != null &&
_migrationStrategyChecker.isExactlyType(node.staticType!))) !=
null;
});

// Additional code copied from the original unawaited_futures.dart lint
// Copyright (c) 2016, 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.
// Source: https://github.com/dart-lang/sdk/blob/main/pkg/linter/lib/src/rules/unawaited_futures.dart

/// A visitor which will report any future that is not awaited.
/// If an [additionalCheck] is provided, it will be used to check if the future should be reported
class _Visitor extends SimpleAstVisitor<void> {
final LintRule rule;
final ErrorReporter reporter;
final LintCode code;
final bool Function(AstNode node) additionalCheck;

_Visitor(this.rule, this.reporter, this.code,
{required this.additionalCheck});

@override
void visitCascadeExpression(CascadeExpression node) {
var sections = node.cascadeSections;
for (var i = 0; i < sections.length; i++) {
_visit(sections[i]);
}
}

@override
void visitExpressionStatement(ExpressionStatement node) {
var expr = node.expression;
if (expr is AssignmentExpression) return;

var type = expr.staticType;
if (type == null) {
return;
}
if (type.implementsInterface('Future', 'dart.async')) {
// Ignore a couple of special known cases.
if (_isFutureDelayedInstanceCreationWithComputation(expr) ||
_isMapPutIfAbsentInvocation(expr)) {
return;
}

if (_isEnclosedInAsyncFunctionBody(node) && additionalCheck(node)) {
// Future expression statement that isn't awaited in an async function:
// while this is legal, it's a very frequent sign of an error.

reporter.atNode(node, code);
}
}
}

@override
void visitInterpolationExpression(InterpolationExpression node) {
_visit(node.expression);
}

bool _isEnclosedInAsyncFunctionBody(AstNode node) {
var enclosingFunctionBody = node.thisOrAncestorOfType<FunctionBody>();
return enclosingFunctionBody?.isAsynchronous ?? false;
}

/// Detects `Future.delayed(duration, [computation])` creations with a
/// computation.
bool _isFutureDelayedInstanceCreationWithComputation(Expression expr) =>
expr is InstanceCreationExpression &&
(expr.staticType?.isDartAsyncFuture ?? false) &&
expr.constructorName.name?.name == 'delayed' &&
expr.argumentList.arguments.length == 2;

bool _isMapClass(Element? e) =>
e is ClassElement && e.name == 'Map' && e.library.name == 'dart.core';

/// Detects Map.putIfAbsent invocations.
bool _isMapPutIfAbsentInvocation(Expression expr) =>
expr is MethodInvocation &&
expr.methodName.name == 'putIfAbsent' &&
// ignore: deprecated_member_use
_isMapClass(expr.methodName.staticElement?.enclosingElement);

void _visit(Expression expr) {
if ((expr.staticType?.isDartAsyncFuture ?? false) &&
_isEnclosedInAsyncFunctionBody(expr) &&
expr is! AssignmentExpression &&
additionalCheck(expr)) {
reporter.atNode(expr, code);
}
}
}

/// The above code snippet depends on some extensions which are copied from
/// https://github.com/dart-lang/sdk/blob/main/pkg/linter/lib/src/extensions.dart
/// The extensions are copied here below:
extension on DartType? {
bool implementsInterface(String interface, String library) {
var self = this;
if (self is! InterfaceType) {
return false;
}
bool predicate(InterfaceType i) => i.isSameAs(interface, library);
var element = self.element;
return predicate(self) ||
!element.isSynthetic && element.allSupertypes.any(predicate);
}

/// Returns whether `this` is the same element as [interface], declared in
/// [library].
bool isSameAs(String? interface, String? library) {
var self = this;
return self is InterfaceType &&
self.element.name == interface &&
self.element.library.name == library;
}
}
20 changes: 20 additions & 0 deletions drift_dev/test/lint/lint_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import 'dart:io';

import 'package:test/test.dart';
import 'package:path/path.dart' as p;

void main() {
// Test the linter_test.dart file
test('linter', () async {
final workingDir = p.join(p.current, 'test/lint/test_pkg');
expect(
await Process.run('dart', ['pub', 'get'], workingDirectory: workingDir)
.then((v) => v.exitCode),
0);
expect(
await Process.run('custom_lint', ['--fatal-infos', '--fatal-warnings'],
workingDirectory: workingDir)
.then((v) => v.exitCode),
0);
});
}
11 changes: 11 additions & 0 deletions drift_dev/test/lint/test_pkg/analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
include: package:lints/recommended.yaml

analyzer:
plugins:
- custom_lint
language:
strict-casts: true

custom_lint:
debug: true
verbose: true
32 changes: 32 additions & 0 deletions drift_dev/test/lint/test_pkg/lib/db.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import 'package:drift/drift.dart';

part 'db.g.dart';

class Users extends Table {
late final id = integer().autoIncrement()();
late final name = text()();
// expect_lint: drift_build_errors
late final age = integer();
}

class BrokenTable extends Table {
// expect_lint: drift_build_errors
IntColumn get unknownRef => integer().customConstraint('CHECK foo > 10')();
}

@DriftDatabase(tables: [Users])
class TestDatabase extends _$TestDatabase {
TestDatabase(super.e);

@override
int get schemaVersion => 1;

a() async {
transaction(
() async {
// expect_lint: unawaited_futures_in_transaction
into(users).insert(UsersCompanion.insert(name: 'name'));
},
);
}
}
312 changes: 312 additions & 0 deletions drift_dev/test/lint/test_pkg/lib/db.g.dart
24 changes: 24 additions & 0 deletions drift_dev/test/lint/test_pkg/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
name: test_pkg
description: A starting point for Dart libraries or applications.
version: 1.0.0
dickermoshe marked this conversation as resolved.
Show resolved Hide resolved

environment:
sdk: ^3.5.3

dependencies:
drift:
build_runner: ^2.4.13

dev_dependencies:
lints: ^4.0.0
test: ^1.24.0
drift_dev:
custom_lint: ^0.6.7

dependency_overrides:
drift:
path: ../../../../drift
drift_dev:
path: ../../../../drift_dev
sqlparser:
path: ../../../../sqlparser
1 change: 1 addition & 0 deletions melos.yaml
Original file line number Diff line number Diff line change
@@ -6,6 +6,7 @@ packages:
- drift
- drift_sqflite
- drift_dev
- drift_dev/test/lint/test_pkg
- drift_flutter
- sqlparser
- examples/*