Migrate newline before return rule and its tests#240
Migrate newline before return rule and its tests#240daria-trusca-solid wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates the newline_before_return lint rule to the new AnalysisRule API, refactoring the rule class and its AST visitor, updating the SDK constraint in pubspec.yaml, and introducing a reflective test suite. Feedback on the changes highlights a potential null pointer exception when accessing the previous token of a return keyword, as well as a bug in the comment-traversal logic within _optimalToken that fails to handle multiple consecutive comments correctly. It is recommended to safely handle null tokens and update the visitor to traverse comment tokens forward, alongside adding a test case to verify multiple comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| static bool _statementHasNewLineBefore( | ||
| ReturnStatement node, | ||
| LineInfo lineInfo, | ||
| ) { | ||
| final previousTokenLineNumber = | ||
| lineInfo.getLocation(node.returnKeyword.previous!.end).lineNumber; | ||
| final root = node.root; | ||
| if (root is! CompilationUnit) return true; | ||
|
|
||
| final lineInfo = root.lineInfo; | ||
|
|
||
| final previousTokenLineNumber = lineInfo | ||
| .getLocation(node.returnKeyword.previous!.end) | ||
| .lineNumber; | ||
| final lastNotEmptyLineToken = _optimalToken(node.returnKeyword, lineInfo); | ||
| final tokenLineNumber = | ||
| lineInfo.getLocation(lastNotEmptyLineToken.offset).lineNumber; | ||
| final tokenLineNumber = lineInfo | ||
| .getLocation(lastNotEmptyLineToken.offset) | ||
| .lineNumber; | ||
|
|
||
| return tokenLineNumber > previousTokenLineNumber + 1; | ||
| } |
There was a problem hiding this comment.
Bug 1: Potential Null Pointer Exception / Crash
In _statementHasNewLineBefore, node.returnKeyword.previous is accessed with the null-assertion operator (!):
final previousTokenLineNumber = lineInfo
.getLocation(node.returnKeyword.previous!.end)
.lineNumber;If previous is null (which can happen in malformed/incomplete code or specific AST structures), this will throw a runtime exception and crash the linter. We should safely handle the null case.
Bug 2: Multiple Comments Bug in _optimalToken
In _optimalToken (defined further down in this file), the loop traverses comments backwards using commentToken.previous:
commentToken = commentToken.previous;However, in the Dart analyzer package, CommentToken does not have a previous pointer to the previous comment token (it is inherited from Token and is null). This means the loop terminates immediately after the first comment token, and any preceding comments are ignored.
As a result, if there are multiple comments above a return statement without a blank line, the rule will incorrectly assume there is a blank line and fail to report a lint.
Recommended Fix for _optimalToken:
Since comment tokens are linked forwards via next, we can collect them into a list and traverse them in reverse order:
static Token _optimalToken(Token token, LineInfo lineInfo) {
var commentToken = token.precedingComments;
if (commentToken == null) return token;
final comments = <Token>[];
var current = commentToken;
while (current != null) {
comments.add(current);
current = current.next;
}
var optimalToken = token;
for (final comment in comments.reversed) {
if (lineInfo.getLocation(comment.end).lineNumber + 1 >=
lineInfo.getLocation(optimalToken.offset).lineNumber) {
optimalToken = comment;
} else {
break;
}
}
return optimalToken;
} static bool _statementHasNewLineBefore(
ReturnStatement node,
) {
final previousToken = node.returnKeyword.previous;
if (previousToken == null) return true;
final root = node.root;
if (root is! CompilationUnit) return true;
final lineInfo = root.lineInfo;
final previousTokenLineNumber = lineInfo
.getLocation(previousToken.end)
.lineNumber;
final lastNotEmptyLineToken = _optimalToken(node.returnKeyword, lineInfo);
final tokenLineNumber = lineInfo
.getLocation(lastNotEmptyLineToken.offset)
.lineNumber;
return tokenLineNumber > previousTokenLineNumber + 1;
}| void test_reports_no_newline_before_return_with_comment() async { | ||
| await assertDiagnostics( | ||
| r''' | ||
| void method() { | ||
| final a = 0; | ||
| // Comment | ||
| return; | ||
| } | ||
| ''', | ||
| [lint(46, 7)], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add a test case to verify that the lint is correctly reported when there are multiple comments above a return statement without a blank line. This test case will fail with the current implementation due to the bug in _optimalToken but will pass once the bug is fixed.
void test_reports_no_newline_before_return_with_comment() async {
await assertDiagnostics(
r'''
void method() {
final a = 0;
// Comment
return;
}
''',
[lint(46, 7)],
);
}
void test_reports_no_newline_before_return_with_multiple_comments() async {
await assertDiagnostics(
r'''
void method() {
final a = 0;
// Comment 1
// Comment 2
return;
}
''',
[lint(63, 7)],
);
}Added test for multiple comments and for a newline before a comment.
Migrated the
newline_before_returnrule and its corresponding tests to comply with the analyzer package.Changes
newline_before_returnto use the current analyzer syntax.