[Parse] Implementation for SE-200 (raw strings) #17668

johnno1962 · 2018-07-02T12:07:33Z

As per discussion in second pitch of raw strings on swift-evolution https://forums.swift.org/t/pure-bikeshedding-raw-strings-why-yes-again/13866. Work in progress, submitting now to get PR link.

Resolves #48912 and also lays groundwork for #51192.

johnno1962 · 2018-08-10T18:16:58Z

@gregomni, any chance you could do a @swift-ci please test for me so I can see what this breaks?

gottesmm · 2018-08-10T19:11:15Z

@swift-ci test

gottesmm · 2018-08-10T19:11:43Z

(just doing a drive by @swift-ci)

johnno1962 · 2018-08-10T19:12:30Z

Cheers.

benrimmington · 2018-08-17T00:14:27Z

test/Parse/raw_string.swift

+/// CHECK: "\\#1"
+
+_ = #"c:\windows\system32"#
+/// CHCECK: "c:\\windows\\system32"


Typo: CHCECK => CHECK

Does it matter how many forward slash characters are used?
i.e. /// CHECK: versus // CHECK:

Would the following also be a valid raw string test?

_ = #""" ################################################################### ## This source file is part of the Swift.org open source project ## ################################################################### """#

johnno1962 · 2018-08-17T00:40:36Z

Tests fixed up and I’ve added the one you suggest and tested.

benrimmington · 2018-08-17T01:09:56Z

test/Parse/raw_string.swift

+// CHECK: "a string with\n\"\"\"\nin it"
+
+_ = #"a raw string containing \r\n"#
+// CHECK "a raw string containing \\r\\n"


Missing colon. // CHECK => // CHECK:

Crikey. Amended.

swift-ci · 2018-09-04T22:10:20Z

Build failed
Swift Test OS X Platform
Git Sha - 999bb40

johnno1962 · 2018-09-06T08:59:58Z

Ready when you are @brentdax, can you run another test please?

beccadax · 2018-09-06T11:21:54Z

@johnno1962 I'll run the source compatibility suite now just to make sure there's no weird parsing issues. Then, in a few hours, I'll smoke test and merge.

beccadax · 2018-09-06T11:22:54Z

@swift-ci please test source compatibility

johnno1962 · 2018-09-06T11:24:39Z

Thanks @brentdax. Don’t forget to squash 👍 Be nice to see this in 4.2.1 ;)

xwu

Some minor comments about comments; nothing major and can be addressed after the fact. Bravo on a great feature and implementation.

xwu · 2018-09-06T12:03:23Z

lib/Parse/Lexer.cpp

+/// the literal beyond what it would appear creating potential security bugs.
+static bool diagnoseZeroWidthMatchAndAdvance(char Target, const char *&CurPtr,
+                                             DiagnosticEngine *Diags) {
+  // Detect, diagnose and skip over zero-width characters here if required.


Marking this as “TODO:” might be a good idea.

xwu · 2018-09-06T12:06:42Z

lib/Parse/Lexer.cpp

+  return 0;
+}
+
+/// delimiterMatches - Does custom delimiter (# characters surrounding quotes)


If you surround “#” with single quotes within this comment, it might alleviate the misread that the custom delimiter is the “number of characters surrounding quotes” (as opposed to the number of ‘#’ characters surrounding—specifically, on either side of—the quotation marks).

xwu · 2018-09-06T12:07:59Z

lib/Parse/Lexer.cpp

+  if (IndentToStrip == ~0u && CustomDelimiterLen == ~0u) {
+    IndentToStrip = CustomDelimiterLen = 0;
+
+    // restore trailing indent removal for multiline


Nit: capitalize and punctuate, here and below.

xwu · 2018-09-06T12:10:36Z

lib/Parse/Lexer.cpp

@@ -1204,6 +1206,68 @@ static bool maybeConsumeNewlineEscape(const char *&CurPtr, ssize_t Offset) {
  }
 }

+/// diagnoseZeroWidthMatchAndAdvance - Error zerowidth characters in delimiters.
+/// A non visible character in the middle of a delimter can be used to extend


Nits: “zero-width” instead of “zerowidth,” “invisible” instead of “non visible.”

Typo in the word “delimiter.”

johnno1962 · 2018-09-06T12:21:26Z

Thanks @xwu, I have the changes ready and will commit after the source compatibility test runs and before @brentdax merges.

johnno1962 · 2018-09-06T13:21:33Z

@brentdax, source compatibility failures look unrelated so I’ve pushed the final nits that xwu mentions.

beccadax · 2018-09-06T17:44:53Z

@johnno1962 Yeah, I saw the same failures on the string interpolation PR.

beccadax · 2018-09-06T17:49:55Z

@swift-ci please smoke test and merge

beccadax · 2018-09-06T18:49:58Z

Ugh, forgot to squash it. I don't think we can clean up history on this repo once it's committed, though.

I guess I've learned a lesson about "smoke test and merge".

johnno1962 · 2018-09-06T19:05:25Z

Thanks Brent. My fault for not squashing it myself but I wanted to keep the history. It’s going to make cherrypicking a bit of a pain though. Does anybody have permission to do a rebase -i on apple/swift to clean it up? Scratch that — the commits are spread over the last few months :(

johnno1962 · 2018-09-06T19:08:21Z

Could you revert it please and I can file the PR again and re-merge? There are a lot of commits.

beccadax · 2018-09-06T19:20:30Z

@johnno1962 Would a revert without actually changing the history meet your goal? I guess if you're just trying to make sure there's one commit to cherry-pick, it would.

johnno1962 · 2018-09-06T19:22:30Z

The single commit with everything in it is what I’m after. I suspect a revert is not going to fix the history.

beccadax · 2018-09-06T20:20:29Z

@johnno1962 Okay, revert is in. Want to resubmit the PR?

johnno1962 · 2018-09-06T20:22:32Z

Trying to at the moment. There is “nothing to compare”. I can start from scratch.

beccadax · 2018-09-06T20:26:42Z

If you revert the revert commit, that should get you the right diff. From there, just fill in the commit message you want.

johnno1962 · 2018-09-06T20:39:58Z

Top tip, thanks. I’ve created a new single commit PR. #19170

rintaro · 2018-09-18T00:15:17Z

lib/Parse/Lexer.cpp

+static bool diagnoseZeroWidthMatchAndAdvance(char Target, const char *&CurPtr,
+                                             DiagnosticEngine *Diags) {
+  // TODO: Detect, diagnose and skip over zero-width characters if required.
+  // See https://github.com/apple/swift/pull/17668 for possible implementation.


@johnno1962 This URL (this PR) doesn't have possible implementation anymore.
Could you file a JIRA issue in https://bugs.swift.org/ . And update the comment?

What do you want me to do here? There is already a Jira SR-8678 for the zero-width problem. Should I raise a new PR to replace the link to the PR with a link to the Jira (which I’ve updated to include the code)? The implementation is under this commit: f0f08e1

Yeah, please 1) add a link to f0f08e1 in SR-8678, and 2) raise a PR to replace the link.

Done: #19364.

Revised implementation for raw strings

14213b8

johnno1962 force-pushed the master branch 5 times, most recently from fd59c17 to 985e2e3 Compare July 8, 2018 16:41

Alternative implementation for raw strings

2317048

johnno1962 force-pushed the master branch from 985e2e3 to 2317048 Compare July 9, 2018 17:39

Delimited/Raw strings inside interpolations

74dd71c

johnno1962 changed the title ~~[Parse][WIP] Revised implementation for raw strings~~ [Parse] Revised implementation for raw strings Aug 10, 2018

johnno1962 force-pushed the master branch 9 times, most recently from 3736766 to 3c5ad62 Compare August 13, 2018 17:59

benrimmington reviewed Aug 17, 2018

View reviewed changes

johnno1962 force-pushed the master branch from 2ea285f to c125f29 Compare August 17, 2018 00:47

benrimmington reviewed Aug 17, 2018

View reviewed changes

Extend token boundary to include delimiter

7866093

johnno1962 force-pushed the master branch 2 times, most recently from b3496fc to 627361c Compare August 17, 2018 15:15

johnno1962 force-pushed the master branch 3 times, most recently from 7ff67f8 to 722b38e Compare September 5, 2018 16:33

Remove zero width detection for now

f0f08e1

johnno1962 force-pushed the master branch from 722b38e to f0f08e1 Compare September 5, 2018 20:51

xwu reviewed Sep 6, 2018

View reviewed changes

Final nits in comments.

9208c5b

swift-ci merged commit 192587c into swiftlang:master Sep 6, 2018

beccadax mentioned this pull request Sep 6, 2018

Revert "[Parse] Implementation for SE-200 (raw strings)" #19168

Merged

johnno1962 mentioned this pull request Sep 6, 2018

Resubmit "[Parse] Implementation for SE-200 (raw strings)" #19170

Merged

rintaro reviewed Sep 18, 2018

View reviewed changes

benlangmuir mentioned this pull request Sep 10, 2018

[SR-8724] Buffer overflow in Lexer::getEncodedStringSegment - Lexer.cpp:2119 #51235

Closed

[Parse] Implementation for SE-200 (raw strings) #17668

[Parse] Implementation for SE-200 (raw strings) #17668

Conversation

johnno1962 commented Jul 2, 2018 • edited by AnthonyLatsis Loading

johnno1962 commented Aug 10, 2018

gottesmm commented Aug 10, 2018

gottesmm commented Aug 10, 2018

johnno1962 commented Aug 10, 2018

Choose a reason for hiding this comment

johnno1962 commented Aug 17, 2018

Choose a reason for hiding this comment

johnno1962 Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

swift-ci commented Sep 4, 2018

johnno1962 commented Sep 6, 2018

beccadax commented Sep 6, 2018

beccadax commented Sep 6, 2018

johnno1962 commented Sep 6, 2018 • edited Loading

xwu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnno1962 commented Sep 6, 2018

johnno1962 commented Sep 6, 2018 • edited Loading

beccadax commented Sep 6, 2018

beccadax commented Sep 6, 2018

beccadax commented Sep 6, 2018

johnno1962 commented Sep 6, 2018 • edited Loading

johnno1962 commented Sep 6, 2018 • edited Loading

beccadax commented Sep 6, 2018

johnno1962 commented Sep 6, 2018

beccadax commented Sep 6, 2018

johnno1962 commented Sep 6, 2018

beccadax commented Sep 6, 2018

johnno1962 commented Sep 6, 2018

Choose a reason for hiding this comment

johnno1962 Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnno1962 commented Jul 2, 2018 •

edited by AnthonyLatsis

Loading

johnno1962 Aug 17, 2018 •

edited

Loading

johnno1962 commented Sep 6, 2018 •

edited

Loading

johnno1962 commented Sep 6, 2018 •

edited

Loading

johnno1962 commented Sep 6, 2018 •

edited

Loading

johnno1962 commented Sep 6, 2018 •

edited

Loading

johnno1962 Sep 18, 2018 •

edited

Loading