Turn deprecation message analyze tests back on (#160554)

Additionally:
- Deprecation message analyzer: use AST more instead of regex 
- Deprecation message analyzer: removes some tests since they are in the
formatter's jurisdiction now
- update the analyzer test fixture to fix a couple line numbers
- `@_debugOnly` set `multiline` back to false. The source we get back
from the `ASTNode` doesn't have any line feed characters.

## Pre-launch Checklist

- [ ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ ] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [ ] I signed the [CLA].
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ ] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This commit is contained in:
LongCatIsLooong 2024-12-27 15:26:24 -08:00 committed by GitHub
parent 840ef1cac8
commit b15625ca92
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 181 additions and 206 deletions

View File

@ -694,18 +694,17 @@ class _DeprecationMessagesVisitor extends RecursiveAstVisitor<void> {
'// flutter_ignore: deprecation_syntax (see analyze.dart)';
/// Some deprecation notices are exempt for historical reasons. They must have an issue listed.
final RegExp legacyDeprecation = RegExp(
r'// flutter_ignore: deprecation_syntax, https://github.com/flutter/flutter/issues/\d+$',
static final RegExp legacyDeprecation = RegExp(
r'// flutter_ignore: deprecation_syntax, https://github.com/flutter/flutter/issues/\d+',
);
final RegExp _deprecationMessagePattern = RegExp(r"^ *'(?<message>.+) '$");
final RegExp _deprecationVersionPattern = RegExp(
r"'This feature was deprecated after v(?<major>\d+)\.(?<minor>\d+)\.(?<patch>\d+)(?<build>-\d+\.\d+\.pre)?\.',?$",
static final RegExp deprecationVersionPattern = RegExp(
r'This feature was deprecated after v(?<major>\d+)\.(?<minor>\d+)\.(?<patch>\d+)(?<build>-\d+\.\d+\.pre)?\.$',
);
String _errorWithLineInfo(AstNode node, {required String error}) {
void _addErrorWithLineInfo(AstNode node, {required String error}) {
final int lineNumber = parseResult.lineInfo.getLocation(node.offset).lineNumber;
return '$filePath:$lineNumber: $error';
errors.add('$filePath:$lineNumber: $error');
}
@override
@ -718,111 +717,107 @@ class _DeprecationMessagesVisitor extends RecursiveAstVisitor<void> {
if (!shouldCheckAnnotation) {
return;
}
final NodeList<StringLiteral> strings;
try {
strings = switch (node.arguments?.arguments) {
null || NodeList<Expression>(first: AdjacentStrings(strings: []), length: 1) =>
throw _errorWithLineInfo(
node,
error:
'@Deprecated annotation should take exactly one string as parameter, got ${node.arguments}',
),
NodeList<Expression>(
first: AdjacentStrings(:final NodeList<StringLiteral> strings),
length: 1,
) =>
strings,
final NodeList<Expression> expressions =>
throw _errorWithLineInfo(
node,
error:
'@Deprecated annotation should take exactly one string as parameter, but got $expressions',
),
};
} catch (error) {
errors.add(error.toString());
final NodeList<Expression>? arguments = node.arguments?.arguments;
if (arguments == null || arguments.length != 1) {
_addErrorWithLineInfo(
node,
error: 'A @Deprecation annotation must have exactly one deprecation notice String.',
);
return;
}
final Expression deprecationNotice = arguments.first;
if (deprecationNotice is! AdjacentStrings) {
_addErrorWithLineInfo(node, error: 'Deprecation notice must be an adjacent string.');
return;
}
final List<StringLiteral> strings = deprecationNotice.strings;
final Iterator<StringLiteral> deprecationMessageIterator = strings.iterator;
final bool isNotEmpty = deprecationMessageIterator.moveNext();
assert(isNotEmpty); // An AdjacentString always has 2 or more string literals.
final [...List<StringLiteral> messageLiterals, StringLiteral versionLiteral] = strings;
// Verify the version literal has the correct pattern.
final RegExpMatch? versionMatch =
versionLiteral is SimpleStringLiteral
? deprecationVersionPattern.firstMatch(versionLiteral.value)
: null;
if (versionMatch == null) {
_addErrorWithLineInfo(
versionLiteral,
error:
'Deprecation notice must end with a line saying "This feature was deprecated after...".',
);
return;
}
final Iterator<StringLiteral> deprecationMessageIterator = strings.iterator;
final bool isNotEmpty = deprecationMessageIterator.moveNext();
assert(isNotEmpty);
try {
RegExpMatch? versionMatch;
String? message;
do {
final StringLiteral deprecationString = deprecationMessageIterator.current;
final String line = deprecationString.toSource();
final RegExpMatch? messageMatch = _deprecationMessagePattern.firstMatch(line);
if (messageMatch == null) {
String possibleReason = '';
if (line.trimLeft().startsWith('"')) {
possibleReason =
' You might have used double quotes (") for the string instead of single quotes (\').';
} else if (!line.contains("'")) {
possibleReason =
' It might be missing the line saying "This feature was deprecated after...".';
} else if (!line.trimRight().endsWith(" '")) {
if (line.contains('This feature was deprecated')) {
possibleReason = ' There might not be an explanatory message.';
} else {
possibleReason = ' There might be a missing space character at the end of the line.';
}
}
throw _errorWithLineInfo(
deprecationString,
error: 'Deprecation notice does not match required pattern.$possibleReason',
);
}
if (message == null) {
message = messageMatch.namedGroup('message');
final String firstChar = String.fromCharCode(message!.runes.first);
if (firstChar.toUpperCase() != firstChar) {
throw _errorWithLineInfo(
deprecationString,
error:
'Deprecation notice should be a grammatically correct sentence and start with a capital letter; see style guide: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md',
);
}
} else {
message += messageMatch.namedGroup('message')!;
}
if (!deprecationMessageIterator.moveNext()) {
throw _errorWithLineInfo(
deprecationString,
error: ' It might be missing the line saying "This feature was deprecated after...".',
);
}
versionMatch = _deprecationVersionPattern.firstMatch(
deprecationMessageIterator.current.toSource(),
);
} while (versionMatch == null);
final int major = int.parse(versionMatch.namedGroup('major')!);
final int minor = int.parse(versionMatch.namedGroup('minor')!);
final int patch = int.parse(versionMatch.namedGroup('patch')!);
final bool hasBuild = versionMatch.namedGroup('build') != null;
// There was a beta release that was mistakenly labeled 3.1.0 without a build.
final bool specialBeta = major == 3 && minor == 1 && patch == 0;
if (!specialBeta && (major > 1 || (major == 1 && minor >= 20))) {
if (!hasBuild) {
throw _errorWithLineInfo(
deprecationMessageIterator.current,
error:
'Deprecation notice does not accurately indicate a beta branch version number; please see https://flutter.dev/docs/development/tools/sdk/releases to find the latest beta build version number.',
);
}
}
if (!message.endsWith('.') && !message.endsWith('!') && !message.endsWith('?')) {
throw _errorWithLineInfo(
node,
final int major = int.parse(versionMatch.namedGroup('major')!);
final int minor = int.parse(versionMatch.namedGroup('minor')!);
final int patch = int.parse(versionMatch.namedGroup('patch')!);
final bool hasBuild = versionMatch.namedGroup('build') != null;
// There was a beta release that was mistakenly labeled 3.1.0 without a build.
final bool specialBeta = major == 3 && minor == 1 && patch == 0;
if (!specialBeta && (major > 1 || (major == 1 && minor >= 20))) {
if (!hasBuild) {
_addErrorWithLineInfo(
versionLiteral,
error:
'Deprecation notice should be a grammatically correct sentence and end with a period; notice appears to be "$message".',
'Deprecation notice does not accurately indicate a beta branch version number; please see https://flutter.dev/docs/development/tools/sdk/releases to find the latest beta build version number.',
);
return;
}
} catch (error) {
errors.add(error.toString());
}
// Verify the version literal has the correct pattern.
assert(messageLiterals.isNotEmpty); // An AdjacentString always has 2 or more string literals.
for (final StringLiteral message in messageLiterals) {
if (message is! SingleStringLiteral) {
_addErrorWithLineInfo(
message,
error: 'Deprecation notice does not match required pattern.',
);
return;
}
if (!message.isSingleQuoted) {
_addErrorWithLineInfo(
message,
error:
'Deprecation notice does not match required pattern. You might have used double quotes (") for the string instead of single quotes (\').',
);
return;
}
}
final String fullExplanation =
messageLiterals
.map((StringLiteral message) => message.stringValue ?? '')
.join()
.trimRight();
if (fullExplanation.isEmpty) {
_addErrorWithLineInfo(
messageLiterals.last,
error:
'Deprecation notice should be a grammatically correct sentence and end with a period; There might not be an explanatory message.',
);
return;
}
final String firstChar = String.fromCharCode(fullExplanation.runes.first);
if (firstChar.toUpperCase() != firstChar) {
_addErrorWithLineInfo(
messageLiterals.first,
error:
'Deprecation notice should be a grammatically correct sentence and start with a capital letter; see style guide: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md',
);
return;
}
if (!fullExplanation.endsWith('.') &&
!fullExplanation.endsWith('?') &&
!fullExplanation.endsWith('!')) {
_addErrorWithLineInfo(
messageLiterals.last,
error:
'Deprecation notice should be a grammatically correct sentence and end with a period; notice appears to be "$fullExplanation".',
);
return;
}
}
}
@ -2339,10 +2334,7 @@ class _DebugOnlyFieldVisitor extends RecursiveAstVisitor<void> {
final List<AstNode> errors = <AstNode>[];
static const String _kDebugOnlyAnnotation = '_debugOnly';
static final RegExp _nullInitializedField = RegExp(
r'kDebugMode \? [\w<> ,{}()]+ : null;',
multiLine: true,
);
static final RegExp _nullInitializedField = RegExp(r'kDebugMode \? [\w<> ,{}()]+ : null;');
@override
void visitFieldDeclaration(FieldDeclaration node) {
@ -2351,7 +2343,7 @@ class _DebugOnlyFieldVisitor extends RecursiveAstVisitor<void> {
(Annotation annotation) => annotation.name.name == _kDebugOnlyAnnotation,
)) {
if (!node.toSource().contains(_nullInitializedField)) {
errors.add(node);
errors.add(node.fields); // Use the fields node for line number.
}
}
}

View File

@ -15,6 +15,14 @@ class Foo {
@_debugOnly
final Map<String, String>? bar = kDebugMode ? null : <String, String>{};
// dart format off
// Checks the annotation works for multiline expressions.
@_debugOnly
final Map<String, String>? multiline = kDebugMode
? <String, String>{}
: null;
// dart format on
}
/// Simply avoid this

View File

@ -11,107 +11,87 @@ void test1() {}
// The code below is intentionally miss-formatted for testing.
// dart format off
@Deprecated(
'Missing space ->.' //ignore: missing_whitespace_between_adjacent_strings
'bad grammar. '
'This feature was deprecated after v1.2.3.'
)
void test2() { }
@Deprecated(
'bad grammar. '
'Also bad grammar '
'This feature was deprecated after v1.2.3.'
)
void test3() { }
@Deprecated(
'Also bad grammar '
'This feature was deprecated after v1.2.3.'
)
void test4() { }
@deprecated // ignore: provide_deprecation_message
void test5() { }
@Deprecated('Not the right syntax. This feature was deprecated after v1.2.3.')
void test6() { }
void test4() { }
@Deprecated(
'Missing the version line. '
)
void test7() { }
void test5() { }
@Deprecated(
'This feature was deprecated after v1.2.3.'
)
void test8() { }
@Deprecated(
'Not the right syntax. '
'This feature was deprecated after v1.2.3.'
) void test9() { }
@Deprecated(
'Not the right syntax. '
'This feature was deprecated after v1.2.3.'
)
void test10() { }
void test6() { }
@Deprecated(
'URLs are not required. '
'This feature was deprecated after v1.0.0.'
)
void test11() { }
void test7() { }
@Deprecated(
'Version number test (should fail). '
'Version number test (should pass). '
'This feature was deprecated after v1.19.0.'
)
void test12() { }
void test8() { }
@Deprecated(
'Version number test (should fail). '
'This feature was deprecated after v1.20.0.'
)
void test13() { }
void test9() { }
@Deprecated(
'Version number test (should fail). '
'This feature was deprecated after v1.21.0.'
)
void test14() { }
void test10() { }
@Deprecated(
'Version number test (special beta should pass). '
'This feature was deprecated after v3.1.0.'
)
void test15() { }
void test11() { }
@Deprecated(
'Version number test (should be fine). '
'This feature was deprecated after v0.1.0.'
)
void test16() { }
void test12() { }
@Deprecated(
'Version number test (should be fine). '
'This feature was deprecated after v1.20.0-1.0.pre.'
)
void test17() { }
void test13() { }
@Deprecated(
"Double quotes' test (should fail). "
'This feature was deprecated after v2.1.0-11.0.pre.'
)
void test18() { }
void test14() { }
@Deprecated( // flutter_ignore: deprecation_syntax, https://github.com/flutter/flutter/issues/000000
'Missing the version line. '
)
void test19() { }
void test15() { }
// dart format on
// flutter_ignore: deprecation_syntax, https://github.com/flutter/flutter/issues/000000
@Deprecated('Missing the version line. ')
void test20() {}
void test16() {}
class TestClass1 {
// flutter_ignore: deprecation_syntax, https://github.com/flutter/flutter/issues/000000

View File

@ -54,51 +54,42 @@ void main() {
);
final String testGenDefaultsPath = path.join('test', 'analyze-gen-defaults');
test(
'analyze.dart - verifyDeprecations',
() async {
final String result = await capture(
() => verifyDeprecations(testRootPath, minimumMatches: 2),
shouldHaveErrors: true,
);
final String lines = <String>[
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:14: Deprecation notice does not match required pattern. There might be a missing space character at the end of the line.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:20: Deprecation notice should be a grammatically correct sentence and start with a capital letter; see style guide: STYLE_GUIDE_URL',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:27: Deprecation notice should be a grammatically correct sentence and end with a period; notice appears to be "Also bad grammar".',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:31: Deprecation notice does not match required pattern.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:34: Deprecation notice does not match required pattern.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:39: Deprecation notice does not match required pattern. It might be missing the line saying "This feature was deprecated after...".',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:43: Deprecation notice does not match required pattern. There might not be an explanatory message.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:50: End of deprecation notice does not match required pattern.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:53: Unexpected deprecation notice indent.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:72: Deprecation notice does not accurately indicate a beta branch version number; please see RELEASES_URL to find the latest beta build version number.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:78: Deprecation notice does not accurately indicate a beta branch version number; please see RELEASES_URL to find the latest beta build version number.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:101: Deprecation notice does not match required pattern. You might have used double quotes (") for the string instead of single quotes (\').',
]
.map((String line) {
return line
.replaceAll('/', Platform.isWindows ? r'\' : '/')
.replaceAll(
'STYLE_GUIDE_URL',
'https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md',
)
.replaceAll(
'RELEASES_URL',
'https://flutter.dev/docs/development/tools/sdk/releases',
);
})
.join('\n');
expect(
result,
'╔═╡ERROR #1╞════════════════════════════════════════════════════════════════════\n'
'$lines\n'
'║ See: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes\n'
'╚═══════════════════════════════════════════════════════════════════════════════\n',
);
},
// TODO(goderbauer): Update and re-enable this after formatting changes have landed.
skip: true,
);
test('analyze.dart - verifyDeprecations', () async {
final String result = await capture(
() => verifyDeprecations(testRootPath, minimumMatches: 2),
shouldHaveErrors: true,
);
final String lines = <String>[
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:14: Deprecation notice should be a grammatically correct sentence and start with a capital letter; see style guide: STYLE_GUIDE_URL',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:20: Deprecation notice should be a grammatically correct sentence and end with a period; notice appears to be "Also bad grammar".',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:25: Deprecation notice must be an adjacent string.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:28: Deprecation notice must be an adjacent string.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:33: Deprecation notice must be an adjacent string.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:52: Deprecation notice does not accurately indicate a beta branch version number; please see RELEASES_URL to find the latest beta build version number.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:58: Deprecation notice does not accurately indicate a beta branch version number; please see RELEASES_URL to find the latest beta build version number.',
'║ test/analyze-test-input/root/packages/foo/deprecation.dart:81: Deprecation notice does not match required pattern. You might have used double quotes (") for the string instead of single quotes (\').',
]
.map((String line) {
return line
.replaceAll('/', Platform.isWindows ? r'\' : '/')
.replaceAll(
'STYLE_GUIDE_URL',
'https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md',
)
.replaceAll(
'RELEASES_URL',
'https://flutter.dev/docs/development/tools/sdk/releases',
);
})
.join('\n');
expect(
result,
'╔═╡ERROR #1╞════════════════════════════════════════════════════════════════════\n'
'$lines\n'
'║ See: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes\n'
'╚═══════════════════════════════════════════════════════════════════════════════\n',
);
});
test('analyze.dart - verifyGoldenTags', () async {
final List<String> result = (await capture(
@ -288,8 +279,15 @@ void main() {
shouldHaveErrors: true,
);
expect(result, contains(':16'));
expect(result, isNot(contains(':13')));
expect(result, isNot(contains(':14')));
expect(result, isNot(contains(':15')));
expect(result, isNot(contains(':19')));
expect(result, isNot(contains(':20')));
expect(result, isNot(contains(':21')));
expect(result, isNot(contains(':22')));
expect(result, contains(':17'));
});
test('analyze.dart - verifyTabooDocumentation', () async {
@ -298,9 +296,9 @@ void main() {
shouldHaveErrors: true,
);
expect(result, isNot(contains(':19')));
expect(result, contains(':20'));
expect(result, contains(':21'));
expect(result, isNot(contains(':27')));
expect(result, contains(':28'));
expect(result, contains(':29'));
});
test('analyze.dart - clampDouble', () async {
@ -313,12 +311,12 @@ void main() {
shouldHaveErrors: true,
);
final String lines = <String>[
'║ packages/flutter/lib/bar.dart:37: input.clamp(0.0, 2)',
'║ packages/flutter/lib/bar.dart:38: input.toDouble().clamp(0, 2)',
'║ packages/flutter/lib/bar.dart:42: nullableInt?.clamp(0, 2.0)',
'║ packages/flutter/lib/bar.dart:43: nullableDouble?.clamp(0, 2)',
'║ packages/flutter/lib/bar.dart:48: nullableInt?.clamp',
'║ packages/flutter/lib/bar.dart:50: nullableDouble?.clamp',
'║ packages/flutter/lib/bar.dart:45: input.clamp(0.0, 2)',
'║ packages/flutter/lib/bar.dart:46: input.toDouble().clamp(0, 2)',
'║ packages/flutter/lib/bar.dart:50: nullableInt?.clamp(0, 2.0)',
'║ packages/flutter/lib/bar.dart:51: nullableDouble?.clamp(0, 2)',
'║ packages/flutter/lib/bar.dart:56: nullableInt?.clamp',
'║ packages/flutter/lib/bar.dart:58: nullableDouble?.clamp',
].map((String line) => line.replaceAll('/', Platform.isWindows ? r'\' : '/')).join('\n');
expect(
result,

View File

@ -377,10 +377,7 @@ bool hasInlineIgnore(
return false;
}
return compilationUnit.content
.substring(
lineInfo.getOffsetOfLine(lineNumber - 1),
lineInfo.getOffsetOfLine(lineNumber) - 1, // Excludes LF, see the comment above.
)
.substring(lineInfo.getOffsetOfLine(lineNumber - 1), lineInfo.getOffsetOfLine(lineNumber))
.trimLeft()
.contains(ignoreDirectivePattern);
}