firka/flutter
firka/flutter
5
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix the sample code analyzer to properly handle missing_identifier errors (#87593)

Browse Source 
This fixes how the sample analyzer handles missing_identifier errors. It was looking at the wrong line, and missing an else clause, so it was silently allowing missing_identifier errors to pass.

In addition, this fixes the sample generation so that it uses the correct filename for the output files: it previously was looking for the first line that had a filename, which was meant to indicate a non-generated line. This change adds a new Line.generated constructor for generated lines, so that they can also have the correct filename associated with them.
This commit is contained in:
Greg Spencer 2021-08-03 17:39:12 -07:00 committed by GitHub
parent cc63c81408
commit 169bb1b73f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 74 additions and 70 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
dev/bots/analyze.dart
View File

@ -109,7 +109,7 @@ Future<void> run(List<String> arguments) async {
// Analyze all the sample code in the repo
print('$clock Sample code...');
await runCommand(dart,
<String>[path.join(flutterRoot, 'dev', 'bots', 'analyze_sample_code.dart'), '--verbose'],
<String>[path.join(flutterRoot, 'dev', 'bots', 'analyze_sample_code.dart')],
workingDirectory: flutterRoot,
);

120
dev/bots/analyze_sample_code.dart
View File

@ -263,7 +263,6 @@ class SampleChecker {
/// Computes the headers needed for each sample file.
List<Line> get headers {
return _headers ??= <String>[
'// generated code',
'// ignore_for_file: directives_ordering',
'// ignore_for_file: unnecessary_import',
'// ignore_for_file: unused_import',
@ -275,12 +274,9 @@ class SampleChecker {
"import 'dart:typed_data';",
"import 'dart:ui' as ui;",
"import 'package:flutter_test/flutter_test.dart';",
for (final File file in _listDartFiles(Directory(_defaultFlutterPackage))) ...<String>[
'',
'// ${file.path}',
for (final File file in _listDartFiles(Directory(_defaultFlutterPackage)))
"import 'package:flutter/${path.basename(file.path)}';",
],
].map<Line>((String code) => Line(code)).toList();
].map<Line>((String code) => Line.generated(code: code, filename: 'headers')).toList();
}
List<Line>? _headers;
@ -495,7 +491,6 @@ class SampleChecker {
} else if (_codeBlockStartRegex.hasMatch(trimmedLine)) {
assert(block.isEmpty);
startLine = Line(
'',
filename: relativeFilePath,
line: lineNumber + 1,
indent: line.indexOf(_dartDocPrefixWithSpace) + _dartDocPrefixWithSpace.length,
@ -507,7 +502,7 @@ class SampleChecker {
final RegExpMatch? sampleMatch = _dartDocSampleBeginRegex.firstMatch(trimmedLine);
if (line == '// Examples can assume:') {
assert(block.isEmpty);
startLine = Line('', filename: relativeFilePath, line: lineNumber + 1, indent: 3);
startLine = Line.generated(filename: relativeFilePath, line: lineNumber + 1, indent: 3);
inPreamble = true;
} else if (sampleMatch != null) {
inSnippet = sampleMatch != null && (sampleMatch[1] == 'sample' || sampleMatch[1] == 'dartpad');
@ -519,7 +514,6 @@ class SampleChecker {
dartpadCount++;
}
startLine = Line(
'',
filename: relativeFilePath,
line: lineNumber + 1,
indent: line.indexOf(_dartDocPrefixWithSpace) + _dartDocPrefixWithSpace.length,
@ -543,7 +537,9 @@ class SampleChecker {
}
}
if (!silent)
print('Found ${sections.length} snippet code blocks, $sampleCount sample code sections, and $dartpadCount dartpad sections.');
print('Found ${sections.length} snippet code blocks, '
'$sampleCount sample code sections, and '
'$dartpadCount dartpad sections.');
for (final Section section in sections) {
final String path = _writeSection(section).path;
if (sectionMap != null)
@ -634,10 +630,10 @@ linter:
final String sectionId = _createNameFromSource('snippet', section.start.filename, section.start.line);
final File outputFile = File(path.join(_tempDirectory.path, '$sectionId.dart'))..createSync(recursive: true);
final List<Line> mainContents = <Line>[
if (section.dartVersionOverride != null) Line(section.dartVersionOverride!) else const Line(''),
Line.generated(code: section.dartVersionOverride ?? '', filename: section.start.filename),
...headers,
const Line(''),
Line('// From: ${section.start.filename}:${section.start.line}'),
Line.generated(filename: section.start.filename),
Line.generated(code: '// From: ${section.start.filename}:${section.start.line}', filename: section.start.filename),
...section.code,
];
outputFile.writeAsStringSync(mainContents.map<String>((Line line) => line.code).join('\n'));
@ -661,10 +657,8 @@ linter:
return line.startsWith('Building flutter tool...');
});
// Check out the stderr to see if the analyzer had it's own issues.
if (stderr.isNotEmpty && (stderr.first.contains(' issues found. (ran in ') || stderr.first.contains(' issue found. (ran in '))) {
// The "23 issues found" message goes onto stderr, which is concatenated first.
if (stderr.isNotEmpty && stderr.first.contains(RegExp(r' issues? found\. \(ran in '))) {
stderr.removeAt(0);
// If there's an "issues found" message, we put a blank line on stdout before it.
if (stderr.isNotEmpty && stderr.last.isEmpty) {
stderr.removeLast();
}
@ -741,7 +735,6 @@ linter:
message,
errorCode,
Line(
'',
filename: file.path,
line: lineNumber,
),
@ -777,7 +770,7 @@ linter:
columnNumber,
message,
errorCode,
Line('', filename: file.path, line: lineNumber),
Line(filename: file.path, line: lineNumber),
),
);
throw SampleCheckerException('Failed to parse error message: $error', file: file.path, line: lineNumber);
@ -793,48 +786,43 @@ linter:
}
final Line actualLine = actualSection.code[lineNumber - 1];
if (actualLine.filename == null) {
late int line;
late int column;
String errorMessage = message;
Line source = actualLine;
if (actualLine.generated) {
// Since generated lines don't appear in the original, we just provide the line
// in the generated file.
line = lineNumber - 1;
column = columnNumber;
if (errorCode == 'missing_identifier' && lineNumber > 1) {
if (fileContents[lineNumber - 2].endsWith(',')) {
final Line actualLine = sections[file.path]!.code[lineNumber - 2];
addAnalysisError(
file,
AnalysisError(
type,
actualLine.line,
actualLine.indent + fileContents[lineNumber - 2].length - 1,
'Unexpected comma at end of sample code.',
errorCode,
actualLine,
),
);
// For a missing identifier on a generated line, it is very often because of a
// trailing comma on the previous line, and so we want to provide a better message
// and the previous line as the error location, since that appears in the original
// source, and can be more easily located.
final Line previousCodeLine = sections[file.path]!.code[lineNumber - 2];
if (previousCodeLine.code.contains(RegExp(r',\s*$'))) {
line = previousCodeLine.line;
column = previousCodeLine.indent + previousCodeLine.code.length - 1;
errorMessage = 'Unexpected comma at end of sample code.';
source = previousCodeLine;
}
} else {
addAnalysisError(
file,
AnalysisError(
type,
lineNumber - 1,
columnNumber,
message,
errorCode,
actualLine,
),
);
}
} else {
addAnalysisError(
file,
AnalysisError(
type,
actualLine.line,
actualLine.indent + columnNumber,
message,
errorCode,
actualLine,
),
);
line = actualLine.line;
column = actualLine.indent + columnNumber;
}
addAnalysisError(
file,
AnalysisError(
type,
line,
column,
errorMessage,
errorCode,
source,
),
);
}
}
if (_exitCode == 1 && analysisErrors.isEmpty && !unknownAnalyzerErrors) {
@ -877,7 +865,7 @@ linter:
// treated as a separate code block.
if (block[index] == '' || block[index] == '// ...') {
if (subline == null)
throw SampleCheckerException('${Line('', filename: line.filename, line: line.line + index, indent: line.indent)}: '
throw SampleCheckerException('${Line(filename: line.filename, line: line.line + index, indent: line.indent)}: '
'Unexpected blank line or "// ..." line near start of subblock in sample code.');
subblocks += 1;
subsections.add(_processBlock(subline, buffer));
@ -889,7 +877,7 @@ linter:
buffer.add('/${block[index]}'); // so that it doesn't start with "// " and get caught in this again
} else {
subline ??= Line(
block[index],
code: block[index],
filename: line.filename,
line: line.line + index,
indent: line.indent,
@ -912,11 +900,17 @@ linter:
/// A class to represent a line of input code.
class Line {
const Line(this.code, {this.filename = 'unknown', this.line = -1, this.indent = 0});
const Line({this.code = '', required this.filename, this.line = -1, this.indent = 0})
: generated = false;
const Line.generated({this.code = '', required this.filename, this.line = -1, this.indent = 0})
: generated = true;
/// The file that this line came from, or the file that the line was generated for, if [generated] is true.
final String filename;
final int line;
final int indent;
final String code;
final bool generated;
String toStringWithColumn(int column) {
if (column != null && indent != null) {
@ -943,7 +937,7 @@ class Section {
for (int i = 0; i < code.length; ++i) {
codeLines.add(
Line(
code[i],
code: code[i],
filename: firstLine.filename,
line: firstLine.line + i,
indent: firstLine.indent,
@ -959,7 +953,7 @@ class Section {
for (int i = 0; i < code.length; ++i) {
codeLines.add(
Line(
code[i],
code: code[i],
filename: firstLine.filename,
line: firstLine.line + i,
indent: firstLine.indent,
@ -967,12 +961,12 @@ class Section {
);
}
return Section(<Line>[
Line(prefix),
Line.generated(code: prefix, filename: firstLine.filename, line: 0),
...codeLines,
Line(postfix),
Line.generated(code: postfix, filename: firstLine.filename, line: 0),
]);
}
Line get start => code.firstWhere((Line line) => line.filename != null);
Line get start => code.firstWhere((Line line) => !line.generated);
final List<Line> code;
final String? dartVersionOverride;

8
dev/bots/test/analyze-sample-code-test-input/known_broken_documentation.dart
View File

@ -113,6 +113,14 @@
/// ```
/// {@end-tool}
///
/// {@tool snippet}
/// snippet with trailing comma
///
/// ```dart
/// const SizedBox(),
/// ```
/// {@end-tool}
///
/// {@tool dartpad --template=stateless_widget_material}
/// Dartpad with null-safe syntax
///

8
dev/bots/test/analyze_sample_code_test.dart
View File

@ -24,7 +24,7 @@ void main() {
..removeWhere((String line) => line.startsWith('Analyzer output:') || line.startsWith('Building flutter tool...'));
expect(process.exitCode, isNot(equals(0)));
expect(stderrLines, <String>[
'In sample starting at dev/bots/test/analyze-sample-code-test-input/known_broken_documentation.dart:117: child: Text(title),',
'In sample starting at dev/bots/test/analyze-sample-code-test-input/known_broken_documentation.dart:125: child: Text(title),',
">>> error: The final variable 'title' can't be read because it is potentially unassigned at this point (read_potentially_unassigned_final)",
'dev/bots/test/analyze-sample-code-test-input/known_broken_documentation.dart:30:9: new Opacity(',
'>>> info: Unnecessary new keyword (unnecessary_new)',
@ -36,12 +36,14 @@ void main() {
'>>> info: Prefer const over final for declarations (prefer_const_declarations)',
'dev/bots/test/analyze-sample-code-test-input/known_broken_documentation.dart:112:25: final int foo = null;',
">>> error: A value of type 'Null' can't be assigned to a variable of type 'int' (invalid_assignment)",
'dev/bots/test/analyze-sample-code-test-input/known_broken_documentation.dart:120:24: const SizedBox(),',
'>>> error: Unexpected comma at end of sample code. (missing_identifier)',
'',
'Found 2 sample code errors.',
''
]);
expect(stdoutLines, <String>[
'Found 8 snippet code blocks, 0 sample code sections, and 2 dartpad sections.',
'Found 9 snippet code blocks, 0 sample code sections, and 2 dartpad sections.',
'Starting analysis of code samples.',
'',
]);
@ -60,7 +62,7 @@ void main() {
expect(process.exitCode, isNot(equals(0)));
expect(stdoutLines, equals(<String>[
// There is one sample code section in the test's dummy dart:ui code.
'Found 8 snippet code blocks, 1 sample code sections, and 2 dartpad sections.',
'Found 9 snippet code blocks, 1 sample code sections, and 2 dartpad sections.',
'',
]));
});

6
packages/flutter/lib/src/painting/strut_style.dart
View File

@ -142,7 +142,7 @@ import 'text_style.dart';
/// fontSize: 30,
/// height: 1.5,
/// ),
/// ),
/// )
/// ```
/// {@end-tool}
///
@ -183,7 +183,7 @@ import 'text_style.dart';
/// fontFamily: 'Roboto',
/// height: 1.5,
/// ),
/// ),
/// )
/// ```
/// {@end-tool}
///
@ -226,7 +226,7 @@ import 'text_style.dart';
/// height: 1,
/// forceStrutHeight: true,
/// ),
/// ),
/// )
/// ```
/// {@end-tool}
///