Skip to content

Commit

Permalink
[rfw] Implement /* block comments */ in the text format (#4769)
Browse files Browse the repository at this point in the history
Also, fix the coverage which previously was not 100% anymore (mostly by making it possible to skip some lines because there's no way to track the coverage on the web side of things as far as I can tell).

Fixes flutter/flutter#133285
  • Loading branch information
Hixie authored Aug 29, 2023
1 parent d9a784e commit d7d3150
Show file tree
Hide file tree
Showing 8 changed files with 169 additions and 19 deletions.
4 changes: 4 additions & 0 deletions packages/rfw/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 1.0.13

* Block comments in RFW's text format. (`/*...*/`)

## 1.0.12

* Improves web compatibility.
Expand Down
7 changes: 7 additions & 0 deletions packages/rfw/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,13 @@ update `test_coverage/bin/test_coverage.dart` with the appropriate
expectations to prevent future coverage regressions. (That program is
run by `run_tests.sh`.)

To run the tests, use `flutter test`.

To run the web tests, use `flutter test --platform=chrome`. If there
is code that only runs on the web target, mark each such line by
ending it with the string `// dead code on VM target`. This will
exclude that line from the coverage calculation.

Golden tests are only run against the Flutter master channel and only
run on Linux, since minor rendering differences are expected on
different platforms and on different versions of Flutter.
16 changes: 8 additions & 8 deletions packages/rfw/lib/src/dart/binary.dart
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,9 @@ class _BlobDecoder {
}
// We use multiplication rather than bit shifts because << truncates to 32 bits when compiled to JS:
// https://dart.dev/guides/language/numbers#bitwise-operations
final int a = bytes.getUint32(byteOffset, _blobEndian);
final int b = bytes.getInt32(byteOffset + 4, _blobEndian);
return a + (b * 0x100000000);
final int a = bytes.getUint32(byteOffset, _blobEndian); // dead code on VM target
final int b = bytes.getInt32(byteOffset + 4, _blobEndian); // dead code on VM target
return a + (b * 0x100000000); // dead code on VM target
}

double _readBinary64() {
Expand Down Expand Up @@ -544,12 +544,12 @@ class _BlobEncoder {
} else {
// We use division rather than bit shifts because >> truncates to 32 bits when compiled to JS:
// https://dart.dev/guides/language/numbers#bitwise-operations
if (value >= 0) {
_scratchIn.setInt32(0, value, _blobEndian);
_scratchIn.setInt32(4, value ~/ 0x100000000, _blobEndian);
if (value >= 0) { // dead code on VM target
_scratchIn.setInt32(0, value, _blobEndian); // dead code on VM target
_scratchIn.setInt32(4, value ~/ 0x100000000, _blobEndian); // dead code on VM target
} else {
_scratchIn.setInt32(0, value, _blobEndian);
_scratchIn.setInt32(4, -((-value) ~/ 0x100000000 + 1), _blobEndian);
_scratchIn.setInt32(0, value, _blobEndian); // dead code on VM target
_scratchIn.setInt32(4, -((-value) ~/ 0x100000000 + 1), _blobEndian); // dead code on VM target
}
}
bytes.add(_scratchOut);
Expand Down
40 changes: 38 additions & 2 deletions packages/rfw/lib/src/dart/text.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ import 'model.dart';
/// * end-of-line comments are supported, using the "//" syntax from C (and
/// Dart).
///
/// * block comments are supported, using the "/*...*/" syntax from C (and
/// Dart).
///
/// * map keys may be unquoted when they are alphanumeric.
///
/// * "null" is not a valid value except as a value in maps, where it is
Expand Down Expand Up @@ -152,8 +155,9 @@ import 'model.dart';
/// binary64 format.
///
/// ```bnf
/// WS ::= ( <U+0020> | <U+000A> | "//" comment* <U+000A or EOF> )
/// WS ::= ( <U+0020> | <U+000A> | "//" comment* <U+000A or EOF> | "/*" blockcomment "*/" )
/// comment ::= <U+0000..U+10FFFF except U+000A>
/// blockcomment ::= <any number of U+0000..U+10FFFF characters not containing the sequence U+002A U+002F>
/// ```
///
/// The `WS` token is used to represent where whitespace and comments are
Expand Down Expand Up @@ -746,6 +750,8 @@ enum _TokenizerMode {
endQuote,
slash,
comment,
blockComment,
blockCommentEnd,
}

String _describeRune(int current) {
Expand Down Expand Up @@ -1133,7 +1139,7 @@ Iterable<_Token> _tokenize(String file) sync* {
case 0x37: // U+0037 DIGIT SEVEN character (7)
case 0x38: // U+0038 DIGIT EIGHT character (8)
case 0x39: // U+0039 DIGIT NINE character (9)
buffer.add(current);
buffer.add(current); // https://github.com/dart-lang/sdk/issues/53349
break;
default:
throw ParserException('Unexpected character ${_describeRune(current)} in integer', line, column);
Expand Down Expand Up @@ -2051,6 +2057,9 @@ Iterable<_Token> _tokenize(String file) sync* {
switch (current) {
case -1:
throw ParserException('Unexpected end of file inside comment delimiter', line, column);
case 0x2A: // U+002A ASTERISK character (*)
mode = _TokenizerMode.blockComment;
break;
case 0x2F: // U+002F SOLIDUS character (/)
mode = _TokenizerMode.comment;
break;
Expand All @@ -2072,6 +2081,33 @@ Iterable<_Token> _tokenize(String file) sync* {
break;
}
break;

case _TokenizerMode.blockComment:
switch (current) {
case -1:
throw ParserException('Unexpected end of file in block comment', line, column);
case 0x2A: // U+002A ASTERISK character (*)
mode = _TokenizerMode.blockCommentEnd;
break;
default:
// ignored, comment
break;
}
break;

case _TokenizerMode.blockCommentEnd:
switch (current) {
case -1:
throw ParserException('Unexpected end of file in block comment', line, column);
case 0x2F: // U+002F SOLIDUS character (/)
mode = _TokenizerMode.main;
break;
default:
// ignored, comment
mode = _TokenizerMode.blockComment;
break;
}
break;
}
index += 1;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/rfw/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: rfw
description: "Remote Flutter widgets: a library for rendering declarative widget description files at runtime."
repository: https://github.com/flutter/packages/tree/main/packages/rfw
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+rfw%22
version: 1.0.12
version: 1.0.13

environment:
sdk: ">=3.0.0 <4.0.0"
Expand Down
8 changes: 8 additions & 0 deletions packages/rfw/test/text_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ void main() {
test('{ "a": /', 'Unexpected end of file inside comment delimiter at line 1 column 8.');
test('{ "a": /.', 'Unexpected character U+002E (".") inside comment delimiter at line 1 column 9.');
test('{ "a": //', 'Unexpected <EOF> at line 1 column 9.');
test('{ "a": /*', 'Unexpected end of file in block comment at line 1 column 9.');
test('{ "a": /*/', 'Unexpected end of file in block comment at line 1 column 10.');
test('{ "a": /**', 'Unexpected end of file in block comment at line 1 column 10.');
test('{ "a": /* *', 'Unexpected end of file in block comment at line 1 column 11.');
});

testWidgets('valid values in parseDataFile', (WidgetTester tester) async {
Expand Down Expand Up @@ -228,6 +232,9 @@ void main() {
expect(parseDataFile('{ "a": \'\\uDDDD\' }'), <String, Object?>{ 'a': '\u{dddd}' });
expect(parseDataFile('{ "a": \'\\uEEEE\' }'), <String, Object?>{ 'a': '\u{eeee}' });
expect(parseDataFile('{ "a": \'\\uFFFF\' }'), <String, Object?>{ 'a': '\u{ffff}' });
expect(parseDataFile('{ "a": /**/ "1" }'), <String, Object?>{ 'a': '1' });
expect(parseDataFile('{ "a": /* */ "1" }'), <String, Object?>{ 'a': '1' });
expect(parseDataFile('{ "a": /*\n*/ "1" }'), <String, Object?>{ 'a': '1' });
});

testWidgets('error handling in parseLibraryFile', (WidgetTester tester) async {
Expand Down Expand Up @@ -294,6 +301,7 @@ void main() {
});

testWidgets('parseLibraryFile: references', (WidgetTester tester) async {
expect(parseLibraryFile('widget a = b(c:data.11234567890."e");').toString(), 'widget a = b({c: data.11234567890.e});');
expect(parseLibraryFile('widget a = b(c: [...for d in []: d]);').toString(), 'widget a = b({c: [...for loop in []: loop0.]});');
expect(parseLibraryFile('widget a = b(c:args.foo.bar);').toString(), 'widget a = b({c: args.foo.bar});');
expect(parseLibraryFile('widget a = b(c:data.foo.bar);').toString(), 'widget a = b({c: data.foo.bar});');
Expand Down
110 changes: 102 additions & 8 deletions packages/rfw/test_coverage/bin/test_coverage.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import 'dart:io';

import 'package:lcov_parser/lcov_parser.dart' as lcov;
import 'package:meta/meta.dart';

// After you run this script, `.../rfw/coverage/lcov.info` will represent the
// latest coverage information for the package. Load that file into your IDE's
Expand All @@ -19,10 +20,32 @@ import 'package:lcov_parser/lcov_parser.dart' as lcov;

// Please update these targets when you update this package.
// Please ensure that test coverage continues to be 100%.
const int targetLines = 3114;
const int targetLines = 3122;
const String targetPercent = '100';
const String lastUpdate = '2023-06-29';

@immutable
/* final */ class LcovLine {
const LcovLine(this.filename, this.line);
final String filename;
final int line;

@override
int get hashCode => Object.hash(filename, line);

@override
bool operator ==(Object other) {
return other is LcovLine &&
other.line == line &&
other.filename == filename;
}

@override
String toString() {
return '$filename:$line';
}
}

Future<void> main(List<String> arguments) async {
// This script is mentioned in the README.md file.

Expand Down Expand Up @@ -54,17 +77,78 @@ Future<void> main(List<String> arguments) async {
exit(0);
}

final List<File> libFiles = Directory('lib')
.listSync(recursive: true)
.whereType<File>()
.where((File file) => file.path.endsWith('.dart'))
.toList();
final Set<LcovLine> flakyLines = <LcovLine>{};
final Set<LcovLine> deadLines = <LcovLine>{};
for (final File file in libFiles) {
int lineNumber = 0;
for (final String line in file.readAsLinesSync()) {
lineNumber += 1;
if (line.endsWith('// dead code on VM target')) {
deadLines.add(LcovLine(file.path, lineNumber));
}
if (line.endsWith('// https://github.com/dart-lang/sdk/issues/53349')) {
flakyLines.add(LcovLine(file.path, lineNumber));
}
}
}

final List<lcov.Record> records = await lcov.Parser.parse(
'coverage/lcov.info',
);
int totalLines = 0;
int coveredLines = 0;
bool deadLinesError = false;
for (final lcov.Record record in records) {
totalLines += record.lines?.found ?? 0;
coveredLines += record.lines?.hit ?? 0;
if (record.lines != null) {
totalLines += record.lines!.found ?? 0;
coveredLines += record.lines!.hit ?? 0;
if (record.file != null && record.lines!.details != null) {
for (int index = 0; index < record.lines!.details!.length; index += 1) {
if (record.lines!.details![index].hit != null &&
record.lines!.details![index].line != null) {
final LcovLine line = LcovLine(
record.file!,
record.lines!.details![index].line!,
);
if (flakyLines.contains(line)) {
totalLines -= 1;
if (record.lines!.details![index].hit! > 0) {
coveredLines -= 1;
}
}
if (deadLines.contains(line)) {
deadLines.remove(line);
totalLines -= 1;
if (record.lines!.details![index].hit! > 0) {
print(
'$line: Line is marked as being dead code but was nonetheless covered.',
);
deadLinesError = true;
}
}
}
}
}
}
}
if (totalLines == 0 || totalLines < coveredLines) {
print('Failed to compute coverage.');
if (deadLines.isNotEmpty || deadLinesError) {
for (final LcovLine line in deadLines) {
print(
'$line: Line is marked as being undetectably dead code but was not considered reachable.',
);
}
print(
'Consider removing the "dead code on VM target" comment from affected lines.',
);
exit(1);
}
if (totalLines <= 0 || totalLines < coveredLines) {
print('Failed to compute coverage correctly.');
exit(1);
}

Expand All @@ -74,7 +158,7 @@ Future<void> main(List<String> arguments) async {
// We only check the TARGET_LINES matches, not the TARGET_PERCENT,
// because we expect the percentage to drop over time as Dart fixes
// various bugs in how it determines what lines are coverable.
if (coveredLines < targetLines) {
if (coveredLines < targetLines && targetLines <= totalLines) {
print('');
print(' ╭──────────────────────────────╮');
print(' │ COVERAGE REGRESSION DETECTED │');
Expand All @@ -100,13 +184,23 @@ Future<void> main(List<String> arguments) async {
} else {
if (coveredLines < totalLines) {
print(
'Warning: Coverage of package:rfw is no longer 100%. (Coverage is now $coveredPercent%.)',
'Warning: Coverage of package:rfw is no longer 100%. (Coverage is now $coveredPercent%, $coveredLines/$totalLines lines.)',
);
}
if (coveredLines > targetLines) {
print(
'test_coverage/bin/test_coverage.dart should be updated to have a new target ($coveredLines).',
'Total lines of covered code has increased, and coverage script is now out of date.\n'
'Coverage is now $coveredPercent%, $coveredLines/$totalLines lines, whereas previously there were only $targetLines lines.\n'
'Update the "\$targetLines" constant at the top of rfw/test_coverage/bin/test_coverage.dart (to $coveredLines).',
);
}
if (targetLines > totalLines) {
print(
'Total lines of code has reduced, and coverage script is now out of date.\n'
'Coverage is now $coveredPercent%, $coveredLines/$totalLines lines, but previously there were $targetLines lines.\n'
'Update the "\$targetLines" constant at the top of rfw/test_coverage/bin/test_coverage.dart (to $totalLines).',
);
exit(1);
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/rfw/test_coverage/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ environment:

dependencies:
lcov_parser: 0.1.1
meta: ^1.7.0

0 comments on commit d7d3150

Please sign in to comment.