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

Issue 167 #184

Closed
wants to merge 4 commits into from
Closed
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.2.4

- Fixed an issue with `prefer_early_retrun` for throw expression

## 0.2.3

- Replace deprecated whereNotNull()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/error/listener.dart';
import 'package:collection/collection.dart';
import 'package:custom_lint_builder/custom_lint_builder.dart';
import 'package:solid_lints/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart';
import 'package:solid_lints/src/models/rule_config.dart';
Expand Down Expand Up @@ -96,7 +95,7 @@ class AvoidReturningWidgetsRule

final isWidgetReturned = hasWidgetType(returnType);

final isIgnored = _shouldIgnore(node);
final isIgnored = config.parameters.exclude.shouldIgnore(node);

final isOverriden = node.declaredElement?.hasOverride ?? false;

Expand All @@ -105,25 +104,4 @@ class AvoidReturningWidgetsRule
}
});
}

bool _shouldIgnore(Declaration node) {
final methodName = node.declaredElement?.name;

final excludedItem = config.parameters.exclude
.firstWhereOrNull((e) => e.methodName == methodName);

if (excludedItem == null) return false;

final className = excludedItem.className;

if (className == null || node is! MethodDeclaration) {
return true;
} else {
final classDeclaration = node.thisOrAncestorOfType<ClassDeclaration>();

if (classDeclaration == null) return false;

return classDeclaration.name.toString() == className;
}
}
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import 'package:solid_lints/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_exclude.dart';
import 'package:solid_lints/src/models/exlude_params.dart';

/// A data model class that represents the "avoid returning widgets" input
/// parameters.
class AvoidReturningWidgetsParameters {
/// A list of methods that should be excluded from the lint.
final List<AvoidReturningWidgetsExclude> exclude;
final ExcludeParameters exclude;

/// Constructor for [AvoidReturningWidgetsParameters] model
AvoidReturningWidgetsParameters({
Expand All @@ -13,16 +13,10 @@ class AvoidReturningWidgetsParameters {

/// Method for creating from json data
factory AvoidReturningWidgetsParameters.fromJson(Map<String, dynamic> json) {
final exclude = <AvoidReturningWidgetsExclude>[];

final excludeList = json['exclude'] as Iterable? ?? [];
for (final item in excludeList) {
if (item is Map) {
exclude.add(AvoidReturningWidgetsExclude.fromJson(item));
}
}
return AvoidReturningWidgetsParameters(
exclude: exclude,
exclude: ExcludeParameters.fromJson(
excludeList: json['exclude'] as Iterable,
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can make this key reusable, so that all other rules use it consistently.

);
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:solid_lints/src/lints/prefer_early_return/visitors/return_statement_visitor.dart';
import 'package:solid_lints/src/lints/prefer_early_return/visitors/throw_expression_visitor.dart';

/// The AST visitor that will collect all unnecessary if statements
class PreferEarlyReturnVisitor extends RecursiveAstVisitor<void> {
Expand Down Expand Up @@ -33,6 +34,7 @@ class PreferEarlyReturnVisitor extends RecursiveAstVisitor<void> {
if (_isElseIfStatement(node)) return;
if (_hasElseStatement(node)) return;
if (_hasReturnStatement(node)) return;
if (_hasThrowExpression(node)) return;

_nodes.add(node);
}
Expand Down Expand Up @@ -70,4 +72,10 @@ class PreferEarlyReturnVisitor extends RecursiveAstVisitor<void> {
node.accept(visitor);
return visitor.nodes.isNotEmpty;
}

bool _hasThrowExpression(Statement node) {
final visitor = ThrowExpressionVisitor();
node.accept(visitor);
return visitor.nodes.isNotEmpty;
}
Comment on lines +75 to +80
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please sync with master -- these changes have already been applied there.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';

/// The AST visitor that will collect every Return statement
class ThrowExpressionVisitor extends RecursiveAstVisitor<void> {
final _nodes = <ThrowExpression>[];

/// All unnecessary return statements
Iterable<ThrowExpression> get nodes => _nodes;

@override
void visitThrowExpression(ThrowExpression node) {
super.visitThrowExpression(node);
_nodes.add(node);
}
}
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
/// Model class for AvoidReturningWidgetsExclude parameters
class AvoidReturningWidgetsExclude {

/// Model class for ExcludeRule parameters
class ExcludeRule {
/// The name of the method that should be excluded from the lint.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's think about better naming here, this isn't a rule.

ExcludedIdentifiersParameter maybe?

Suggested change
class ExcludeRule {
class ExcludeRule {

final String methodName;

/// The name of the class that should be excluded from the lint.
final String? className;

/// Constructor for [AvoidReturningWidgetsExclude] model
const AvoidReturningWidgetsExclude({
/// Constructor for [ExcludeRule] model
const ExcludeRule({
required this.methodName,
required this.className,
});

///
factory AvoidReturningWidgetsExclude.fromJson(
factory ExcludeRule.fromJson(
Map<dynamic, dynamic> json,
) {
return AvoidReturningWidgetsExclude(
return ExcludeRule(
methodName: json['method_name'] as String,
className: json['class_name'] as String?,
);
Expand Down
53 changes: 53 additions & 0 deletions lib/src/models/exlude_params.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:collection/collection.dart';
import 'package:solid_lints/src/models/exclude_rule.dart';

/// A data model class that represents the "avoid returning widgets" input
/// parameters.
class ExcludeParameters {
/// A list of methods that should be excluded from the lint.
final List<ExcludeRule> exclude;

/// Constructor for [ExcludeParameters] model
ExcludeParameters({
required this.exclude,
});

/// Method for creating from json data
factory ExcludeParameters.fromJson({
required Iterable<dynamic> excludeList,
}) {
final exclude = <ExcludeRule>[];

for (final item in excludeList) {
if (item is Map) {
exclude.add(ExcludeRule.fromJson(item));
}
}
return ExcludeParameters(
exclude: exclude,
);
}

/// Method to define ignoring
bool shouldIgnore(Declaration node) {
final methodName = node.declaredElement?.name;

final excludedItem =
exclude.firstWhereOrNull((e) => e.methodName == methodName);

if (excludedItem == null) return false;

final className = excludedItem.className;

if (className == null || node is! MethodDeclaration) {
return true;
} else {
final classDeclaration = node.thisOrAncestorOfType<ClassDeclaration>();

if (classDeclaration == null) return false;

return classDeclaration.name.toString() == className;
}
}
}
62 changes: 62 additions & 0 deletions lint_test/prefer_early_return_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,65 @@ void threeSeqentialIfReturn2() {
_doSomething();
}
}

void oneIfWithThrowWithReturn() {
//no lint
if (true) {
throw '';
}

return;
}

void oneIfElseWithThrowReturn() {
//no lint
if (true) {
_doSomething();
} else {
throw '';
}
}

void twoSeqentialIfWithThrow() {
if (false) throw '';
//expect_lint: prefer_early_return
if (true) {
_doSomething();
}
}

void twoSeqentialIfWithThrowReturn2() {
//no lint
if (false) throw '';
//expect_lint: prefer_early_return
if (true) {
_doSomething();
}

return;
}

void threeSeqentialIfWithThrowReturn() {
//no lint
if (false) throw '';
if (true) throw '';
//expect_lint: prefer_early_return
if (true) {
_doSomething();
}

return;
}

void threeSeqentialIfWithThrowReturn2() {
//no lint
if (false) throw '';
//no lint
if (true) {
_doSomething();
}
//expect_lint: prefer_early_return
if (true) {
_doSomething();
}
}
Loading