[clang-format] Disable string breaking in JS for now #66372
Merged
+17
−132
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See the discussion [here](llvm#66168 (comment)). The functionality is not mature enough.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-format ChangesSee the discussion [here](https://github.com//pull/66168#issuecomment-1719038797).The functionality is not mature enough.Full diff: https://github.com/llvm/llvm-project/pull/66372.diff 4 Files Affected:
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index d5d9faa5c78cffb..4ab0b3a207270dc 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -2783,17 +2783,17 @@ the configuration (without a prefix: ``Auto``).
const char* x =
"veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongString";
- In C#, Java, and JavaScript:
+ In C# and Java:
.. code-block:: c++
true:
- var x = "veryVeryVeryVeryVeryVe" +
- "ryVeryVeryVeryVeryVery" +
- "VeryLongString";
+ string x = "veryVeryVeryVeryVeryVe" +
+ "ryVeryVeryVeryVeryVery" +
+ "VeryLongString";
false:
- var x =
+ string x =
"veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongString";
C# and JavaScript interpolated strings are not broken.
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index c7bd356b7faeded..3bf70838d059086 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -2062,15 +2062,15 @@ struct FormatStyle {
/// "veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongString";
/// \endcode
///
- /// In C#, Java, and JavaScript:
+ /// In C# and Java:
/// \code
/// true:
- /// var x = "veryVeryVeryVeryVeryVe" +
- /// "ryVeryVeryVeryVeryVery" +
- /// "VeryLongString";
+ /// string x = "veryVeryVeryVeryVeryVe" +
+ /// "ryVeryVeryVeryVeryVery" +
+ /// "VeryLongString";
///
/// false:
- /// var x =
+ /// string x =
/// "veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongString";
/// \endcode
///
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 6673b5c703b835f..15b9ad41a37af59 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -2237,15 +2237,10 @@ ContinuationIndenter::createBreakableToken(const FormatToken &Current,
LineState &State, bool AllowBreak) {
unsigned StartColumn = State.Column - Current.ColumnWidth;
if (Current.isStringLiteral()) {
- // Strings in JSON can not be broken.
- if (Style.isJson() || !Style.BreakStringLiterals || !AllowBreak)
- return nullptr;
-
- // Strings in TypeScript types and dictionary keys can not be broken.
- if (Style.isJavaScript() &&
- (Current.is(TT_SelectorName) ||
- State.Line->startsWith(Keywords.kw_type) ||
- State.Line->startsWith(tok::kw_export, Keywords.kw_type))) {
+ // Strings in JSON can not be broken. Breaking strings in JavaScript is
+ // disabled for now.
+ if (Style.isJson() || Style.isJavaScript() || !Style.BreakStringLiterals ||
+ !AllowBreak) {
return nullptr;
}
diff --git a/clang/unittests/Format/FormatTestJS.cpp b/clang/unittests/Format/FormatTestJS.cpp
index 51543d0a54d8561..23b010dbc982684 100644
--- a/clang/unittests/Format/FormatTestJS.cpp
+++ b/clang/unittests/Format/FormatTestJS.cpp
@@ -1506,121 +1506,11 @@ TEST_F(FormatTestJS, StringLiteralConcatenation) {
verifyFormat("var literal = 'hello ' +\n"
" 'world';");
- // Long strings should be broken.
+ // String breaking is disabled for now.
verifyFormat("var literal =\n"
- " 'xxxxxxxx ' +\n"
- " 'xxxxxxxx';",
+ " 'xxxxxxxx xxxxxxxx';",
"var literal = 'xxxxxxxx xxxxxxxx';",
getGoogleJSStyleWithColumns(17));
- verifyFormat("var literal =\n"
- " 'xxxxxxxx ' +\n"
- " 'xxxxxxxx';",
- "var literal = 'xxxxxxxx xxxxxxxx';",
- getGoogleJSStyleWithColumns(18));
- verifyFormat("var literal =\n"
- " 'xxxxxxxx' +\n"
- " ' xxxxxxxx';",
- "var literal = 'xxxxxxxx xxxxxxxx';",
- getGoogleJSStyleWithColumns(16));
- // The quotes should be correct.
- for (char OriginalQuote : {'\'', '"'}) {
- auto VerifyQuotes = [=](FormatStyle::JavaScriptQuoteStyle StyleQuote,
- char TargetQuote) {
- auto Style = getGoogleJSStyleWithColumns(17);
- Style.JavaScriptQuotes = StyleQuote;
- std::string Target{"var literal =\n"
- " \"xxxxxxxx \" +\n"
- " \"xxxxxxxx\";"};
- std::string Original{"var literal = \"xxxxxxxx xxxxxxxx\";"};
- std::replace(Target.begin(), Target.end(), '"', TargetQuote);
- std::replace(Original.begin(), Original.end(), '"', OriginalQuote);
- verifyFormat(Target, Original, Style);
- };
- VerifyQuotes(FormatStyle::JSQS_Leave, OriginalQuote);
- VerifyQuotes(FormatStyle::JSQS_Single, '\'');
- VerifyQuotes(FormatStyle::JSQS_Double, '"');
- }
- // Parentheses should be added when necessary.
- verifyFormat("var literal =\n"
- " ('xxxxxxxx ' +\n"
- " 'xxxxxx')[0];",
- "var literal = 'xxxxxxxx xxxxxx'[0];",
- getGoogleJSStyleWithColumns(18));
- auto Style = getGoogleJSStyleWithColumns(20);
- Style.SpacesInParens = FormatStyle::SIPO_Custom;
- Style.SpacesInParensOptions.Other = true;
- verifyFormat("var literal =\n"
- " ( 'xxxxxxxx ' +\n"
- " 'xxxxxx' )[0];",
- "var literal = 'xxxxxxxx xxxxxx'[0];", Style);
- // FIXME: When the part before the string literal is shorter than the
- // continuation indentation, and the option AlignAfterOpenBracket is set to
- // AlwaysBreak which is the default for the Google style, the unbroken string
- // does not get to a new line while the broken string does due to the added
- // parentheses. The formatter does not do it in one pass.
- EXPECT_EQ(
- "x = ('xxxxxxxx ' +\n"
- " 'xxxxxx')[0];",
- format("x = 'xxxxxxxx xxxxxx'[0];", getGoogleJSStyleWithColumns(18)));
- verifyFormat("x =\n"
- " ('xxxxxxxx ' +\n"
- " 'xxxxxx')[0];",
- getGoogleJSStyleWithColumns(18));
- // Breaking of template strings and regular expressions is not implemented.
- verifyFormat("var literal =\n"
- " `xxxxxxxx xxxxxxxx`;",
- getGoogleJSStyleWithColumns(18));
- verifyFormat("var literal =\n"
- " /xxxxxxxx xxxxxxxx/;",
- getGoogleJSStyleWithColumns(18));
- // There can be breaks in the code inside a template string.
- verifyFormat("var literal = `xxxxxx ${\n"
- " xxxxxxxxxx} xxxxxx`;",
- "var literal = `xxxxxx ${xxxxxxxxxx} xxxxxx`;",
- getGoogleJSStyleWithColumns(14));
- verifyFormat("var literal = `xxxxxx ${\n"
- " xxxxxxxxxx} xxxxxx`;",
- "var literal = `xxxxxx ${xxxxxxxxxx} xxxxxx`;",
- getGoogleJSStyleWithColumns(15));
- // Identifiers inside the code inside a template string should not be broken
- // even if the column limit is exceeded. This following behavior is not
- // optimal. The part after the closing brace which exceeds the column limit
- // can be put on a new line. Change this test when it is implemented.
- verifyFormat("var literal = `xxxxxx ${\n"
- " xxxxxxxxxxxxxxxxxxxxxx} xxxxxx`;",
- "var literal = `xxxxxx ${xxxxxxxxxxxxxxxxxxxxxx} xxxxxx`;",
- getGoogleJSStyleWithColumns(14));
- verifyFormat("var literal = `xxxxxx ${\n"
- " xxxxxxxxxxxxxxxxxxxxxx +\n"
- " xxxxxxxxxxxxxxxxxxxxxx} xxxxxx`;",
- "var literal = `xxxxxx ${xxxxxxxxxxxxxxxxxxxxxx + "
- "xxxxxxxxxxxxxxxxxxxxxx} xxxxxx`;",
- getGoogleJSStyleWithColumns(14));
-
- // Strings in a TypeScript type declaration can't be broken.
- verifyFormat("type x =\n"
- " 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx';",
- getGoogleJSStyleWithColumns(20));
- verifyFormat("/* type */ type x =\n"
- " 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx';",
- getGoogleJSStyleWithColumns(20));
- verifyFormat("export type x =\n"
- " 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx';",
- getGoogleJSStyleWithColumns(20));
- // Dictionary keys can't be broken. Values can be broken.
- verifyFormat("var w = {\n"
- " 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx':\n"
- " 'xxxxxxxxxx' +\n"
- " 'xxxxxxxxxx' +\n"
- " 'xxxxxxxxxx' +\n"
- " 'xxxxxxxxxx' +\n"
- " 'xxx',\n"
- "};",
- "var w = {\n"
- " 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx':\n"
- " 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',\n"
- "};",
- getGoogleJSStyleWithColumns(20));
}
TEST_F(FormatTestJS, RegexLiteralClassification) {
|
alexfh
approved these changes
Sep 15, 2023
There was a problem hiding this comment.
This is the right call for now. There are multiple contexts in JS, TS and libraries like Closure, where simple string literals are expected. Getting all of these right may require some back-and-forths, during which clang-format will continue producing invalid code. Let's get back to a good state and figure out details after that.
owenca
approved these changes
Sep 15, 2023
|
|
||
| false: | ||
| var x = | ||
| string x = | ||
| "veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongString"; | ||
|
|
||
| C# and JavaScript interpolated strings are not broken. |
There was a problem hiding this comment.
Suggested change
| C# and JavaScript interpolated strings are not broken. | |
| C# interpolated strings are not broken. |
| (Current.is(TT_SelectorName) || | ||
| State.Line->startsWith(Keywords.kw_type) || | ||
| State.Line->startsWith(tok::kw_export, Keywords.kw_type))) { | ||
| // Strings in JSON can not be broken. Breaking strings in JavaScript is |
There was a problem hiding this comment.
Suggested change
| // Strings in JSON can not be broken. Breaking strings in JavaScript is | |
| // Strings in JSON cannot be broken. Breaking strings in JavaScript is |
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
See the discussion [here](llvm#66168 (comment)). The functionality is not mature enough.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See the discussion
here.
The functionality is not mature enough.