Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
102 changes: 63 additions & 39 deletions protobuf/lib/src/protobuf/mixins/well_known.dart
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ String _typeNameFromUrl(String typeUrl) {
}

mixin TimestampMixin {
static final RegExp finalGroupsOfThreeZeroes = RegExp(r'(?:000)*$');
static final RegExp finalGroupsOfThreeZeroes = RegExp(r'(?:000)+$');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(Only used for replacing with '', no need to match an empty string.)


Int64 get seconds;
set seconds(Int64 value);
Expand All @@ -192,9 +192,15 @@ mixin TimestampMixin {
///
/// Time zone information will not be preserved.
static void setFromDateTime(TimestampMixin target, DateTime dateTime) {
final micros = dateTime.microsecondsSinceEpoch;
target.seconds = Int64((micros / Duration.microsecondsPerSecond).floor());
target.nanos = (micros % Duration.microsecondsPerSecond).toInt() * 1000;
var micros = dateTime.microsecondsSinceEpoch;
var seconds = micros ~/ Duration.microsecondsPerSecond;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Going through doubles loses precision for large/small dates. Tests added.

micros = micros.remainder(Duration.microsecondsPerSecond);
if (micros < 0) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(Usual way to handle ~//remainder not working like %.)

seconds -= 1;
micros += Duration.microsecondsPerSecond;
}
target.seconds = Int64(seconds);
target.nanos = micros * 1000;
}

static String _twoDigits(int n) {
Expand Down Expand Up @@ -268,10 +274,10 @@ mixin TimestampMixin {
JsonParsingContext context,
) {
if (json is String) {
var jsonWithoutFracSec = json;
var nanos = 0;
final Match? fracSecsMatch = RegExp(r'\.(\d+)').firstMatch(json);
if (fracSecsMatch != null) {
var jsonWithoutFracSec = json.replaceFirstMapped(RegExp(r'\.(\d+)'), (

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(Could combine match and replace.)

fracSecsMatch,
) {
final fracSecs = fracSecsMatch[1]!;
if (fracSecs.length > 9) {
throw context.parseException(
Expand All @@ -280,12 +286,8 @@ mixin TimestampMixin {
);
}
nanos = int.parse(fracSecs.padRight(9, '0'));
jsonWithoutFracSec = json.replaceRange(
fracSecsMatch.start,
fracSecsMatch.end,
'',
);
}
return '';
});
final dateTimeWithoutFractionalSeconds =
DateTime.tryParse(jsonWithoutFracSec) ??
(throw context.parseException(
Expand Down Expand Up @@ -319,16 +321,30 @@ mixin DurationMixin {
TypeRegistry typeRegistry,
) {
final duration = message as DurationMixin;
final secFrac = duration.nanos
// nanos and seconds should always have the same sign.
final seconds = duration.seconds;
final nanos = duration.nanos;
// Extra sign needed if `seconds` is zero and nanos is negative.
final sign = seconds.isZero && nanos < 0 ? '-' : '';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was a bug. It wrote 0 seconds and -500000000 nanos as 0.5.


return '$sign$seconds${_toNanosString(nanos)}s';
}

/// Nanoseconds as fractional seconds.
///
/// Is empty if [nanos] is zero, otherwise the digits of
/// (absolute value) of `nanos` as billionths of seconds
/// with no trailing zeros.
static String _toNanosString(int nanos) {
if (nanos == 0) return '';
final digits = nanos
.abs()
.toString()
.padLeft(9, '0')
.replaceFirst(finalZeroes, '');
final secPart = secFrac == '' ? '' : '.$secFrac';
return '${duration.seconds}${secPart}s';
return '.$digits';
}

// Matches `-?\d*\.?\d*s$`, grouping is only to control captures.
static final RegExp durationPattern = RegExp(r'(-?\d*)(?:\.(\d*))?s$');

static void fromProto3JsonHelper(
Expand All @@ -340,25 +356,33 @@ mixin DurationMixin {
final duration = message as DurationMixin;
if (json is String) {
final match = durationPattern.matchAsPrefix(json);
if (match == null) {
throw context.parseException(
'Expected a String of the form `<seconds>.<nanos>s`',
json,
);
} else {
if (match != null) {
final secondsString = match[1]!;
final seconds =
secondsString == '' ? Int64.ZERO : Int64.parseInt(secondsString);
duration.seconds = seconds;
final nanos = int.parse((match[2] ?? '').padRight(9, '0'));
duration.nanos = seconds < 0 ? -nanos : nanos;
final isNegative = secondsString.startsWith('-');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was a bug in the opposite direction.
It parsed -0.5 as 0 seconds and 500000000 nanos, and didn't negate the nanos since 0 was not negative.

duration.seconds =
(secondsString.length == (isNegative ? 1 : 0))
? Int64.ZERO
: Int64.parseInt(secondsString);
var fractionalSeconds = match[2];
var nanos = 0;
if (fractionalSeconds != null) {
if (fractionalSeconds.length > 9) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The original code would parse 0.9999999999 as 9+ seconds.

I assume the input would be correctly generated and not have more than 9 digits, but better to be safe.
(Could also throw.)

// Shouldn't happen for valid input.
fractionalSeconds = fractionalSeconds.substring(0, 9);
} else {
fractionalSeconds = fractionalSeconds.padRight(9, '0');
}
nanos = int.parse(fractionalSeconds);
if (isNegative) nanos = -nanos;
}
duration.nanos = nanos;
return;
}
} else {
throw context.parseException(
'Expected a String of the form `<seconds>.<nanos>s`',
json,
);
}
throw context.parseException(
'Expected a String of the form `<seconds>.<nanos>s`',
json,
);
}
}

Expand Down Expand Up @@ -565,7 +589,7 @@ mixin FieldMaskMixin {
) {
final fieldMask = message as FieldMaskMixin;
for (final path in fieldMask.paths) {
if (path.contains(RegExp('[A-Z]|_[^a-z]'))) {
if (path.contains(RegExp(r'[A-Z]|_[^a-z]'))) {
throw ArgumentError(
'Bad fieldmask $path. Does not round-trip to json.',
);
Expand All @@ -587,7 +611,7 @@ mixin FieldMaskMixin {
json,
);
}
if (json == '') {
if (json.isEmpty) {
// The empty string splits to a single value. So this is a special case.
return;
}
Expand All @@ -604,15 +628,15 @@ mixin FieldMaskMixin {

static String _toCamelCase(String name) {
return name.replaceAllMapped(
RegExp('_([a-z])'),
(Match m) => m.group(1)!.toUpperCase(),
RegExp(r'_[a-z]'),
(Match m) => name[m.start + 1].toUpperCase(),
);
}

static String _fromCamelCase(String name) {
return name.replaceAllMapped(
RegExp('[A-Z]'),
(Match m) => '_${m.group(0)!.toLowerCase()}',
RegExp(r'[A-Z]'),
(Match m) => '_${m[0]!.toLowerCase()}',
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,9 @@ class Duration extends $pb.GeneratedMessage with $mixin.DurationMixin {
static Duration fromDart($core.Duration duration) => Duration()
..seconds = $fixnum.Int64(duration.inSeconds)
..nanos =
(duration.inMicroseconds % $core.Duration.microsecondsPerSecond) * 1000;
duration.inMicroseconds
.remainder($core.Duration.microsecondsPerSecond) *
1000;
}

const $core.bool _omitFieldNames =
Expand Down
4 changes: 2 additions & 2 deletions protoc_plugin/lib/names.dart
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ Iterable<String> extensionSuffixes() sync* {
///
/// This function does not take care of leading underscores.
String legalDartIdentifier(String input) {
return input.replaceAll(RegExp(r'[^a-zA-Z0-9$_]'), '_');
return input.replaceAll(RegExp(r'[^\w$]'), '_');
}

/// Chooses the name of the Dart class holding top-level extensions.
Expand Down Expand Up @@ -592,7 +592,7 @@ String? _nameOption(FieldDescriptorProto field) =>

bool _isDartFieldName(String name) => name.startsWith(_dartFieldNameExpr);

final _dartFieldNameExpr = RegExp(r'^[a-z]\w+$');
final _dartFieldNameExpr = RegExp(r'^[a-z]\w*$');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(Seems unlikely that single-letter fields are not allowed, but two-letter ones are.)


/// Names that would collide as top-level identifiers.
final List<String> forbiddenTopLevelNames = <String>[
Expand Down
2 changes: 1 addition & 1 deletion protoc_plugin/lib/src/file_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

part of '../protoc.dart';

final RegExp _dartIdentifier = RegExp(r'^\w+$');
final RegExp _dartIdentifier = RegExp(r'^[a-zA-Z_]\w*$');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That didn't match Dart identifiers, it allowed starting with 0.
(Also still doesn't allow $s, but I assume that's deliberate.)


const String _asyncImportUrl = 'dart:async';

Expand Down
28 changes: 10 additions & 18 deletions protoc_plugin/lib/src/protobuf_field.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,10 @@ part of '../protoc.dart';

class ProtobufField {
static final RegExp _hexLiteralRegex = RegExp(
r'^0x[0-9a-f]+$',
multiLine: false,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

multiLine: false is the default.
(And I think being explicit is more readable than the caseSensitive: false flag.)

caseSensitive: false,
r'^0x[\da-fA-F]+$',
);
static final RegExp _integerLiteralRegex = RegExp(r'^[+-]?[0-9]+$');
static final RegExp _decimalLiteralRegexA = RegExp(
r'^[+-]?([0-9]*)\.[0-9]+(e[+-]?[0-9]+)?$',
multiLine: false,
caseSensitive: false,
);
static final RegExp _decimalLiteralRegexB = RegExp(
r'^[+-]?[0-9]+e[+-]?[0-9]+$',
multiLine: false,
caseSensitive: false,
static final RegExp _decimalLiteralRegex = RegExp(
r'^[+\-]?(\d*\.)?\d+([eE][+\-]?\d+)?$',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Instead of three regexpsm just use one and switch on whether it has double-literal parts.

);

final FieldDescriptorProto descriptor;
Expand Down Expand Up @@ -411,10 +401,12 @@ class ProtobufField {
return '$coreImportPrefix.double.nan';
} else if (_hexLiteralRegex.hasMatch(descriptor.defaultValue)) {
return '(${descriptor.defaultValue}).toDouble()';
} else if (_integerLiteralRegex.hasMatch(descriptor.defaultValue)) {
return '${descriptor.defaultValue}.0';
} else if (_decimalLiteralRegexA.hasMatch(descriptor.defaultValue) ||
_decimalLiteralRegexB.hasMatch(descriptor.defaultValue)) {
} else if (_decimalLiteralRegex.matchAsPrefix(descriptor.defaultValue)
case final match?) {
if (match[1] == null && match[2] == null) {
// Integer literal, no `.` or `e` parts.
return '${descriptor.defaultValue}.0';
}
return descriptor.defaultValue;
}
throw _invalidDefaultValue;
Expand Down Expand Up @@ -502,7 +494,7 @@ class ProtobufField {
static String _unCamelCase(String name) {
return name.replaceAllMapped(
_upperCase,
(match) => '_${match.group(0)!.toLowerCase()}',
(match) => '_${match[0]!.toLowerCase()}',
);
}
}
Expand Down
20 changes: 10 additions & 10 deletions protoc_plugin/lib/src/shared.dart
Original file line number Diff line number Diff line change
Expand Up @@ -71,25 +71,25 @@ String? toDartComment(String value) {

if (value.isEmpty) return null;

var lines = LineSplitter.split(value).toList();
var lines = LineSplitter.split(value)
.map((line) => line.trimRight())
.toList();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Doing trimRight early makes later tests easier, it turns all lines with only whitespace into empty lines.


// Find any leading spaces in the first line. If all of the lines have the
// same leading spaces, remove them all.
final leadingSpaces = _leadingSpaces.firstMatch(lines.first);
if (leadingSpaces != null) {
final prefix = leadingSpaces.group(0)!;
final prefix = leadingSpaces[0]!;
if (lines.every((line) => line.isEmpty || line.startsWith(prefix))) {
lines =
lines
.map(
(line) => line.isEmpty ? line : line.substring(prefix.length),
)
.toList();
for (var i = 0; i < lines.length; i++) {
var line = lines[i];
if (line.isNotEmpty) lines[i] = line.substring(prefix.length);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just update the list in-place instead of building a new one.

}
}

// Remove empty, trailing lines.
while (lines.isNotEmpty && lines.last.trim().isEmpty) {
while (lines.isNotEmpty && lines.last.isEmpty) {
lines.removeLast();
}

Expand All @@ -98,7 +98,7 @@ String? toDartComment(String value) {
return null;
}

return lines.map((e) => '/// $e'.trimRight()).join('\n');
return lines.map((e) => e.isEmpty ? '///' : '/// $e').join('\n');
}

final _leadingSpaces = RegExp('^ +');
5 changes: 4 additions & 1 deletion protoc_plugin/lib/src/well_known_types.dart
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ $coreImportPrefix.Duration toDart() =>
/// Creates a new instance from [$coreImportPrefix.Duration].
static Duration fromDart($coreImportPrefix.Duration duration) => Duration()
..seconds = $fixnumImportPrefix.Int64(duration.inSeconds)
..nanos = (duration.inMicroseconds % $coreImportPrefix.Duration.microsecondsPerSecond) * 1000;
..nanos =
duration.inMicroseconds
.remainder($coreImportPrefix.Duration.microsecondsPerSecond) *
1000;
''',
],
wellKnownType: 'duration',
Expand Down
3 changes: 3 additions & 0 deletions protoc_plugin/test/duration_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ void main() {
);

expect(pb.Duration.fromDart(coreDuration).toDart(), coreDuration);

final coreDuration2 = Duration(milliseconds: -500);
expect(pb.Duration.fromDart(coreDuration2).toDart(), coreDuration2);
});

test('proto duration -> core duration -> proto duration', () {
Expand Down
32 changes: 31 additions & 1 deletion protoc_plugin/test/proto3_json_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ void main() {
predicate((e) {
if (e is FormatException) {
final pathExpression =
RegExp(r'root(\["[^"]*"]*\])*').firstMatch(e.message)![0]!;
RegExp(r'root(?:\["[^"]*"\])*').firstMatch(e.message)![0]!;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(I think the extra ]* here was a typo. If it had been deliberate, it would have been escaped.
Since it matches the empty string, nobody noticed that you could match root["a"]]]]].

final actualPath =
RegExp(
r'\["([^"]*)"\]',
Expand Down Expand Up @@ -1230,6 +1230,12 @@ void main() {
..seconds = Int64(10)
..nanos = 10,
);
expect(
Duration()..mergeFromProto3Json('10.000000001999s'),
Duration()
..seconds = Int64(10)
..nanos = 1,
);
expect(
Duration()..mergeFromProto3Json('-1.000099s'),
Duration()
Expand All @@ -1255,6 +1261,18 @@ void main() {
..seconds = Int64(0)
..nanos = 500000000,
);
expect(
Duration()..mergeFromProto3Json('-.5s'),
Duration()
..seconds = Int64(0)
..nanos = -500000000,
);
expect(
Duration()..mergeFromProto3Json('-0.0s'),
Duration()
..seconds = Int64(0)
..nanos = 0,
);
expect(
Duration()..mergeFromProto3Json('5.s'),
Duration()
Expand All @@ -1267,6 +1285,18 @@ void main() {
..seconds = Int64(0)
..nanos = 0,
);
expect(
Duration()..mergeFromProto3Json('-s'),
Duration()
..seconds = Int64(0)
..nanos = 0,
);
expect(
Duration()..mergeFromProto3Json('s'),
Duration()
..seconds = Int64(0)
..nanos = 0,
);
expect(() => Duration()..mergeFromProto3Json('0.5'), parseFailure([]));
expect(() => Duration()..mergeFromProto3Json(100), parseFailure([]));
});
Expand Down
Loading