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

new lint: invalid_case_patterns #4047

Merged
merged 9 commits into from
Feb 8, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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 @@ -77,6 +77,7 @@ linter:
- hash_and_equals
- implementation_imports
- implicit_call_tearoffs
- invalid_case_patterns
- iterable_contains_unrelated_type
- join_return_with_assignment
- leading_newlines_in_multiline_strings
Expand Down
2 changes: 2 additions & 0 deletions lib/src/rules.dart
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import 'rules/flutter_style_todos.dart';
import 'rules/hash_and_equals.dart';
import 'rules/implementation_imports.dart';
import 'rules/implicit_call_tearoffs.dart';
import 'rules/invalid_case_patterns.dart';
import 'rules/invariant_booleans.dart';
import 'rules/iterable_contains_unrelated_type.dart';
import 'rules/join_return_with_assignment.dart';
Expand Down Expand Up @@ -308,6 +309,7 @@ void registerLintRules({bool inTestMode = false}) {
..register(ImplementationImports())
..register(ImplicitCallTearoffs())
..register(InvariantBooleans())
..register(InvalidCasePatterns())
..register(IterableContainsUnrelatedType())
..register(JoinReturnWithAssignment())
..register(LeadingNewlinesInMultilineStrings())
Expand Down
92 changes: 92 additions & 0 deletions lib/src/rules/invalid_case_patterns.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// Copyright (c) 2023, 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';

const _desc = r'Use case expressions that are valid in Dart 3.0.';

/// todo(pq): add details or link out to a doc?
Copy link
Member

Choose a reason for hiding this comment

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

First, that's a false dichotomy; we could have both.

But I think the answer to the question of whether to have more details here depends on how well the doc we should link to (so "yes" to the second question) does of explaining the behavior of this lint. If the doc provides important details about switch statements, but doesn't directly address the breaking changes, then we need details here. If the doc is dedicated to the breaking changes, then we might not need any extra details here.

But either way, we need to provide the kind of before and after examples that we have in the other diagnostic documentation in order to help users figure out how to migrate their code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is the kind of input you're looking for @pq, but hopefully it helps.

I'm imagining that within the umbrella of patterns documentation in dart.dev, there will be a reference on switch cases, what works, what doesn't work, the relationship/significance of switch for patterns, etc.

Or, that the language tour reference on switch will document the new valid and invalid case patterns, agnostic of patterns, but that points to a page about the relationship between switch and patterns.

If the doc provides important details about switch statements, but doesn't directly address the breaking changes, then we need details here.

@bwilkerson I'm not sure what "directly addressing the breaking changes" on dart.dev would mean, like explicitly calling out that this is effective from 3.0+? Or that the result of these invalid cases is a compile time error?

...depends on how well the doc we should link to...does of explaining the behavior of this lint.

If I understand correctly, "explaining the behavior of the lint" is a combination of:

  • explaining the relationship between patterns and switch (why)
  • the advent of that change, i.e. Dart 3 (when)
  • showing everything that's invalid (what)
  • alongside what is valid/what should be done instead (how).

Basically documenting #4044 in dart.dev. Would that satisfy documenting the lint "well"?

I'm not against more detail in the lint message, but I am biased towards more complete dart.dev content. So, if fleshing it out on dart.dev solves the problem of making this lint message really long, then I'm happy to make sure the doc it links to instead is a sufficient resource.

Also important to consider....

Patterns documentation will definitely be up by Dart 3, but I currently don't have a timeline more granular than that, AND it won't actually be published to the website before release day. So that's something to consider, if you need a working link before then. I could prioritize that work, or at the very least create the page and section to link to if the cutoff is very soon.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what "directly addressing the breaking changes" on dart.dev would mean ...

The lint is designed to catch expressions in pre-3.0 switch case clauses whose semantics will change, without a diagnostic, when the code is migrated to 3.0. Those are the breaking changes I'm referring to.

If I understand correctly, "explaining the behavior of the lint" is a combination of: ...

That's exactly what I meant by "directly addressing" the breaking changes.

Would that satisfy documenting the lint "well"?

Yes, that would be perfect.

Patterns documentation will definitely be up by Dart 3 ...

The lint will, hopefully, ship with the next beta, so we'll want to have documentation explaining what the lint is for before we ship 3.0. We can always change the docs later to be reduced and to point to the dart.dev page, but I think it's important for users to understand how this helps them prepare for 3.0 sooner than the release of 3.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @MaryaBelanger! In the short term, I agree w/ @bwilkerson that we should make sure that the lint has docs that are complete enough that folks who try it out aren't totally lost. In the medium term, it would be great to link out to a more complete doc on dart.dev (and what you describe sounds perfect). The good news is the linter docs are auto-generated so it's easy for us to update and refresh them once the doc we want to link to goes live.

Copy link
Contributor

@MaryaBelanger MaryaBelanger Feb 8, 2023

Choose a reason for hiding this comment

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

Aha, with that timeline it seems like the lint docs are the best place to hold all that info (I think you both already knew that, I'm catching up haha). I'm not sure if you were looking for input on the lint itself, but now that I'm thinking about it...

I might be missing something, but since the correction message says "Try refactoring" you'd want to provide them with a solution or explanation for each potential case right? Unless you don't typically explain lint topics that come with a quick fix (but there's a few expressions that don't have a quick fix so the question still stands for those).

To avoid having to write out every expression's GOOD/BAD example, would it be possible to add a string token to the correction message that's populated based on the expression that the linter flagged? Please excuse my horrifying dart/pseudocode mashup, but something like:

class InvalidCasePatterns extends LintRule {
  static const LintCode code = LintCode('invalid_case_patterns',
      "This expression is not valid in a 'case' clause in Dart 3.0.",
      correctionMessage: 'Try refactoring the expression to be valid in 3.0:' token);
...
    } else if (expression is ParenthesizedExpression) {
      token = "discard parenthesis";
      rule.reportLint(expression);
    }

Otherwise I'd imagine the lint details could just list out all of the affected expressions, maybe splitting them up by "compile time error/syntactically breaking" and "behavior/semantically breaking". Again, a messy mashup but just for reference, something like:

BAD:

switch (obj) {
// syntactic error
  case {1}: // Set literal.
  case (1): // Parenthesized expression.
  case identical(1, 2): // `identical()` call.
  case -pi: // Unary operator.
  case 1 + 2: // Binary operator.
  case true ? 1 : 2: // Conditional operator.
  case 'hi'.length: // .length call.
  case i is int: // is expression.

// semantic error
  case [1, 2]: // list literal
  case {'k': 'v'}:  // map literal
  case Point(1, 2): // constant constructor call
  case _: // wildcard
  case true && false: // logic operators
}

GOOD:

switch (obj) {

// List literals, map literals, set literals, and constant constructor calls:
// Put const before the literal or call:

  case const [1, 2]:
  case const {'k': 'v'}:
  case const {1, 2}:
  case const Point(1, 2):

// ... etc. for the rest

Maybe you've already considered all of this 😅 I'm still not 100% sure how to read a lint rule's source. Anyway, I'll let you know when the Dart 3 docs start coming together and where you can link to once the content is available.

Copy link
Member Author

Choose a reason for hiding this comment

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

I might be missing something, but since the correction message says "Try refactoring" you'd want to provide them with a solution or explanation for each potential case right?

You're not missing anything. Where we can, I think we should provide a more specific message. (Often we do and here we should for sure.) TL;DR you're 💯 spot on.

would it be possible to add a string token to the correction message that's populated based on the expression that the linter flagged?

Yep. Possible!

I'm not sure if you were looking for input on the lint itself, but now that I'm thinking about it...

Always. So thank you!

const _details = r'''
Some case expressions that are valid in Dart 2.19 and below will become an error
or have changed semantics when a library is upgraded to 3.0. This lint flags
those expressions in order to ease migration to Dart 3.0.
''';

class InvalidCasePatterns extends LintRule {
static const LintCode code = LintCode(
'invalid_case_patterns', 'Invalid case pattern.',
pq marked this conversation as resolved.
Show resolved Hide resolved
// todo(pq): can we have a doc link here?
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing that you mean "can we link directly to some doc other than the one generated from the _details. In which case the answer is that it isn't currently supported.

I think the link to the other doc should be in the _details. There will automatically be a link in the IDE to the doc page generated from the _details. That might seem like a bit of an indirection, but it's also more consistent with the way the rest of the IDE links work, and I think that consistency trumps the extra indirection.

correctionMessage: 'Try refactoring the expression to be valid in 3.0.');

InvalidCasePatterns()
: super(
name: 'invalid_case_patterns',
description: _desc,
details: _details,
group: Group.errors);

@override
LintCode get lintCode => code;

@override
void registerNodeProcessors(
NodeLintRegistry registry, LinterContext context) {
var visitor = _Visitor(this);
registry.addSwitchCase(this, visitor);
}
}

class _Visitor extends SimpleAstVisitor {
final LintRule rule;

_Visitor(this.rule);

@override
visitSwitchCase(SwitchCase node) {
var expression = node.expression;
if (expression is SetOrMapLiteral) {
rule.reportLint(expression);
} else if (expression is ListLiteral) {
rule.reportLint(expression);
} else if (expression is ParenthesizedExpression) {
rule.reportLint(expression);
} else if (expression is MethodInvocation) {
if (expression.methodName.isDartCoreIdentifier(named: 'identical')) {
rule.reportLint(expression);
}
} else if (expression is PrefixExpression) {
rule.reportLint(expression);
} else if (expression is BinaryExpression) {
rule.reportLint(expression);
} else if (expression is ConditionalExpression) {
rule.reportLint(expression);
} else if (expression is PropertyAccess) {
if (expression.propertyName.isDartCoreIdentifier(named: 'length')) {
rule.reportLint(expression);
}
} else if (expression is IsExpression) {
rule.reportLint(expression);
} else if (expression is InstanceCreationExpression) {
if (expression.isConst) {
rule.reportLint(expression);
}
} else if (expression is SimpleIdentifier) {
var token = expression.token;
if (token is StringToken && token.lexeme == '_') {
rule.reportLint(expression);
}
}
}
}

extension on SimpleIdentifier {
bool isDartCoreIdentifier({required String named}) {
if (name != named) return false;
var library = staticElement?.library;
return library != null && library.isDartCore;
}
}
2 changes: 2 additions & 0 deletions test/rules/all.dart
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import 'discarded_futures_test.dart' as discarded_futures;
import 'file_names_test.dart' as file_names;
import 'flutter_style_todos_test.dart' as flutter_style_todos;
import 'hash_and_equals_test.dart' as hash_and_equals;
import 'invalid_case_patterns_test.dart' as invalid_case_patterns;
import 'library_annotations_test.dart' as library_annotations;
import 'library_names_test.dart' as library_names;
import 'library_private_types_in_public_api_test.dart'
Expand Down Expand Up @@ -126,6 +127,7 @@ void main() {
file_names.main();
flutter_style_todos.main();
hash_and_equals.main();
invalid_case_patterns.main();
library_annotations.main();
library_names.main();
library_private_types_in_public_api.main();
Expand Down
182 changes: 182 additions & 0 deletions test/rules/invalid_case_patterns_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
// Copyright (c) 2023, 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:test_reflective_loader/test_reflective_loader.dart';

import '../rule_test_support.dart';

main() {
defineReflectiveSuite(() {
defineReflectiveTests(InvalidCasePatternsTest);
});
}

@reflectiveTest
class InvalidCasePatternsTest extends LintRuleTest {
@override
String get lintRule => 'invalid_case_patterns';

test_binaryExpression() async {
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case 1 + 2:
}
}
''', [
lint(43, 5),
]);
}

test_binaryExpression_logic() async {
await assertDiagnostics(r'''
f(bool b) {
switch (b) {
case true && false:
}
}
''', [
lint(36, 13),
]);
}

test_conditionalExpression() async {
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case true ? 1 : 2:
}
}
''', [
lint(43, 12),
error(HintCode.DEAD_CODE, 54, 1),
]);
}

test_constConstructorCall() async {
pq marked this conversation as resolved.
Show resolved Hide resolved
await assertDiagnostics(r'''
class C {
const C();
}

f(C c) {
switch (c) {
case C():
}
}
''', [
lint(59, 3),
]);
}

test_identicalCall() async {
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case identical(1, 2):
}
}
''', [
lint(43, 15),
]);
}

test_isExpression() async {
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case 1 is int:
}
}
''', [
error(HintCode.UNNECESSARY_TYPE_CHECK_TRUE, 45, 8),
lint(45, 8),
]);
}

test_lengthCall() async {
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case ''.length:
}
}
''', [
lint(43, 9),
]);
}

test_listLiteral() async {
pq marked this conversation as resolved.
Show resolved Hide resolved
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case [1, 2]:
}
}
''', [
lint(43, 6),
]);
}

test_mapLiteral() async {
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case {'k': 'v'}:
}
}
''', [
lint(42, 10),
]);
}

test_parenthesizedExpression() async {
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case (1):
pq marked this conversation as resolved.
Show resolved Hide resolved
}
}
''', [
lint(43, 3),
]);
}

test_prefixedExpression() async {
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case -1:
pq marked this conversation as resolved.
Show resolved Hide resolved
}
}
''', [
lint(43, 2),
]);
}

test_setLiteral() async {
await assertDiagnostics(r'''
void f(Object o) {
switch (o) {
case {1}:
}
}
''', [
lint(43, 3),
]);
}

test_wildcard() async {
await assertDiagnostics(r'''
f(int n) {
const _ = 3;
switch (n) {
case _:
}
}
''', [
lint(50, 1),
]);
}
}