Allow single quotes in "raw" string literals#2357
Conversation
Closes #2351 Closes #2350 I cannot find evidence of use case for writing a string literal that includes interpolation. Drop the generality and update the doc to describe a behavior where the content of the resulting string always matches the content of the argument. Use an implementation from `package:source_helper` that attempts to choose an idiomatic delimiter and falls back to single quotes with every character escaped. Bump major version since this is technically breaking. Most team usages should be able to use an expanded constraint.
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with
Unused Dependencies
|
| Package | Status |
|---|---|
| code_builder | ❗ Show IssuesThese packages may be unused, or you may be using assets from these packages: |
For details on how to fix these, see dependency_validator.
This check can be disabled by tagging the PR with skip-unused-dependencies-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
This check can be disabled by tagging the PR with skip-changelog-check.
Coverage ⚠️
| File | Coverage |
|---|---|
| pkgs/code_builder/lib/src/specs/expression/literal.dart | 💔 78 % ⬇️ 3 % |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
Breaking changes ✔️
| Package | Change | Current Version | New Version | Needed Version | Looking good? |
|---|---|---|---|---|---|
| code_builder | None | 4.11.1 | 4.12.0-wip | 4.12.0-wip | ✔️ |
This check can be disabled by tagging the PR with skip-breaking-check.
API leaks ✔️
The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
| Package | Leaked API symbol | Leaking sources |
|---|
This check can be disabled by tagging the PR with skip-leaking-check.
|
Ugh, I forgot that |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a breaking change to literalString, removing the raw parameter in favor of an automated escaping mechanism that handles single and double quotes, dollar signs, and control characters. The package version is updated to 5.0.0-wip. Review feedback highlights logic errors in the newly added test cases regarding the expected output for strings with dollar signs. Suggestions were also provided to avoid variable shadowing in the escaping logic and to optimize performance by defining the escape map regular expression as a constant.
Avoid a dependency cycle with `package:build_runner` during the major version bump. We won't be able to regenerate `built_value` classes until we update build runner for compatibility
|
@davidmorgan - this change will impact Since mockito is google3 SoT I plan to:
Do you have any concerns? |
|
After a brief discussion, I'm now leaning towards making this an incremental migration. It will require two major version bumps, but it will allow compatibility with google3 during all phases.
|
Make the migration more incremental
natebosch
changed the title
|
/gemini review (just to bother @natebosch ) |
There was a problem hiding this comment.
Code Review
This pull request updates the literalString function to support single quotes when raw: true is used, implementing a logic that chooses between single/double quotes and raw/non-raw formats. It includes a version bump to 4.12.0-wip and comprehensive tests. Feedback suggests removing a redundant backslash entry in the escape map and using a more robust Unicode escape format for hex literals.
| '\f': r'\f', // 0C - form feed | ||
| '\r': r'\r', // 0D - carriage return | ||
| '\x7F': r'\x7F', // delete | ||
| r'\': r'\\', // backslash |
There was a problem hiding this comment.
The entry for the backslash character in _escapeMap is redundant because backslashes are explicitly handled in the replaceAllMapped callback within _escapeString (lines 75-78). Removing this entry would improve maintainability and avoid confusion about how backslashes are processed.
| String _hexLiteral(String input) { | ||
| final value = input.runes.single | ||
| .toRadixString(16) | ||
| .toUpperCase() | ||
| .padLeft(2, '0'); | ||
| return '\\x$value'; | ||
| } |
There was a problem hiding this comment.
The _hexLiteral function currently uses \x and pads to 2 digits, which is only valid for 8-bit characters (up to \xFF). While the current _escapeRegExp only matches characters within this range, this implementation is fragile if the regex is expanded in the future to include non-ASCII characters. Consider using the more robust \u{...} format which supports all Unicode code points in Dart.
String _hexLiteral(String input) {
final value = input.runes.single
.toRadixString(16)
.toUpperCase();
return '\\u{$value}';
}
|
I tried this w/ my json_serializable migration and it an issue with After hacking I got this diff which made things work. diff --git a/pkgs/code_builder/lib/src/specs/expression/literal.dart b/pkgs/code_builder/lib/src/specs/expression/literal.dart
index 1ec0136a..98ee4121 100644
--- a/pkgs/code_builder/lib/src/specs/expression/literal.dart
+++ b/pkgs/code_builder/lib/src/specs/expression/literal.dart
@@ -48,7 +48,9 @@ Expression literalNum(num value) => LiteralExpression._('$value');
/// or double quotes, and may not actually be marked raw, depending on the
/// content. All disallowed characters are automatically escaped.
Expression literalString(String value, {bool raw = false}) {
- if (raw) return LiteralExpression._(_escapeString(value));
+ if (raw || value.contains(r'$')) {
+ return LiteralExpression._(_escapeString(value));
+ }
final escaped = value.replaceAll('\'', '\\\'').replaceAll('\n', '\\n');
return LiteralExpression._("'$escaped'");
}
diff --git a/pkgs/code_builder/test/specs/code/expression_test.dart b/pkgs/code_builder/test/specs/code/expression_test.dart
index 6ac88571..46643f9a 100644
--- a/pkgs/code_builder/test/specs/code/expression_test.dart
+++ b/pkgs/code_builder/test/specs/code/expression_test.dart
@@ -30,6 +30,9 @@ void main() {
test('string values', () {
expect(literal('foo'), equalsDart("'foo'"));
});
+ test('string values containing \$ should use raw strings', () {
+ expect(literal(r'$schema'), equalsDart(r"r'$schema'"));
+ });
test('list values', () {
expect(literal([1]), equalsDart('[1]'));
});
@@ -63,7 +66,7 @@ void main() {
group('literalString legacy', () {
test('should emit a String', () {
- expect(literalString(r'$monkey'), equalsDart(r"'$monkey'"));
+ expect(literalString(r'$monkey'), equalsDart(r"r'$monkey'"));
});
test('should emit a raw String', () { |
Mockito is not google3-first any more, it's synced as needed github->google3 by me. The script that I'm not super convinced about the idea a |
That change works exactly against the incremental migration goals. We want to keep the behavior of
I have already adjusted the PR to an incremental migration approach. Luckily we can avoid the ugly naming. I'll keep that in mind and update mockito in the build repo after this starts landing. We can discuss timeline for the major version bump once that PR is ready. Impacted packages can use wide bounds across the two migrations |
|
Sounds good, thanks. I went ahead and removed the code_builder dep from build_runner, working on rolling into google3. So code_builders and individual builders are more free to do as they like :) |
In #2357 we added more complex escaping to allow creating safe string literals even when they may include single quote characters. The new implementation didn't prefer raw strings by default so use existing uses changed behavior and some tests which hardcode expected generated output a non-behavior impacting diff appears. Prefer to use actual prefixed raw strings when there are no single quotes. Existing callers passing strings allowed by the old implementation will not be impacted by the change. In the next steps of the migration calling sites will only see a behavior difference as they change their arguments. In the end all tests will be migrated to the code path which doesn't prefer raw prefixed strings, but the incremental changes will only impact tests when library code is changing.
3 participants
Closes #2352
Closes #2350
The raw string behavior is more useful for codegen, but it disallows
single quotes in the content which is too limiting. Expand the behavior
to always create an allowed string literal with exactly the same content
as the argument and allow single quotes. The literal is no longer
guaranteed to be an actual raw string tagged with
r, but there are nobehavior differences and this matches the intent for the argument.
A future breaking change will change the default behavior of the method
to match this new
raw: truebehavior. Changing the current behaviorwith the argument allows for an incremental migration. There are not
dependencies on the existing behavior which guarantees the
rprefix. Asubsequent breaking change will remove the argument altogether.