Accept underscores in unicode escapes #43716

MaloJaffre · Aug 7, 2017

I don't know if this need an RFC, but at least the impl is here!

rust-highfive · Aug 7, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

kennytm · Aug 7, 2017

kennytm reviewed Aug 7, 2017

View changes

kennytm · Aug 7, 2017

src/libsyntax/parse/lexer/mod.rs

    ///
-    /// At this point, we have already seen the \ and the u, the { is the current character. We
-    /// will read at least one digit, and up to 6, and pass over the }.
+    /// At this point, we have already seen the `\` and the `u`, the `{` is the current character. We


The CI is unhappy with this line because it is too long 🙂

[00:03:14] tidy error: /checkout/src/libsyntax/parse/lexer/mod.rs:968: line longer than 100 chars [00:03:15] some tidy checks failed

kennytm · Aug 7, 2017

issue-43692.rs should be a run-pass test, not a compile-fail test. It should check (assert_eq) if '\u{10__FFFF}' and "\u{10_F0FF}foo\u{1_0_0_0}" are equivalent to their no-underscore counterparts.

arielb1 · Aug 8, 2017

r? @petrochenkov

petrochenkov · Aug 9, 2017

@SimonSapin what do you think about this?

Also ping @rust-lang/lang
This probably doesn't need a full RFC, but will certainly need an FCP.

SimonSapin · Aug 9, 2017

Making numeric escape sequences consistent with integer literals makes sense to me. 👍

joshtriplett · Aug 9, 2017

Please don't allow prefixes or suffixes, but otherwise this seems like a great idea.

petrochenkov · Aug 12, 2017

@MaloJaffre
Could you somehow share the lexing code between unicode escapes and normal hexadecimal literals to ensure the rules are identical?
For example, scan_digits can be reused for unicode escapes.
Unicode escapes are more restrictive, but the restrictions could be enforced after a unicode escape is lexed (this can also give better error reporting and recovery).

MaloJaffre · Aug 13, 2017

Thanks for the suggestion @petrochenkov.
I've also rebased on master.

Edit: Travis failure looks spurious (workers failed to start)

petrochenkov · Aug 17, 2017

petrochenkov reviewed Aug 17, 2017

View changes

petrochenkov · Aug 17, 2017

src/libsyntax/parse/lexer/mod.rs

+        loop {
+            match self.ch {
+                Some('}') => {
+                    if valid && count == 0 {


if count == 0 would give the same result

No, because in the case \u{#}, we don't want to say that the escape is empty, so we check there was no invalid characters before.

Ah, right, this is in a loop, okay then.

petrochenkov · Aug 17, 2017

petrochenkov reviewed Aug 17, 2017

View changes

petrochenkov · Aug 17, 2017

src/libsyntax/parse/lexer/mod.rs

+                        self.err_span_char(start_bpos,
+                                           self.pos,
+                                           "invalid character in unicode escape",
+                                           c);


This error can now be reported a lot of times in case of unterminated unicode escapes.
It probably should be reported only the first time.

petrochenkov · Aug 17, 2017

petrochenkov reviewed Aug 17, 2017

View changes

petrochenkov · Aug 17, 2017

src/libsyntax/parse/mod.rs

+                            diag.struct_span_err(span, "invalid unicode character escape")
+                                .help("unicode escape must be at most 10FFFF")
+                                .emit();
+                                None


I think you can avoid an changing the return type to option here and just return something like Replacement character U+FFFD.

@MaloJaffre
Could you also squash commits after updating the PR?

Thanks for the review @petrochenkov!

Ok, I will shortly do another round of changes and squash everything.

petrochenkov · Aug 17, 2017

Implementation LGTM, modulo comments.

@rfcbot fcp merge

petrochenkov · Aug 17, 2017

I have no rights for @rfcbot, could someone start an FCP?

MaloJaffre · Aug 17, 2017

@petrochenkov Done.
I've also added a more precise help message for surrogates.

Edit: Travis failure looks spurious (OSX jobs failed to start).

petrochenkov · Aug 17, 2017

petrochenkov approved these changes Aug 17, 2017

View changes

MaloJaffre · Aug 25, 2017

Friendly ping @nikomatsakis, to start a FCP, if there are no concerns about the implementation.

petrochenkov · Aug 25, 2017

@MaloJaffre
There's one more thing that I forgot about - this needs a feature gate (unless the lang team decides it doesn't), #[feature(unicode_escape_underscores)] or something.
See 50ecee2 for an example of how to add it.
"Parse session" is available from the lexer, so it shouldn't be a problem (I think).

SimonSapin · Aug 25, 2017

I think this is fine without a feature gate. (Though I’m not in any team that would make that decision.)

aturon · Aug 25, 2017

@rfcbot fcp merge

rfcbot · Aug 25, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

pnkfelix · Aug 30, 2017

pnkfelix reviewed Aug 30, 2017

View changes

pnkfelix · Aug 30, 2017

src/libsyntax/parse/lexer/mod.rs

-            self.bump();
-            count += 1;
+        if let Some('_') = self.ch {
+            // disallow leading `_`


do we need a compile-fail test checking that leading _ is disallowed?

There is already a parse-fail test that checks that, do I need to move it to compile-fail?

rfcbot · Sep 1, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · Sep 11, 2017

The final comment period is now complete.

petrochenkov · Sep 11, 2017

The final comment period is now complete.

@bors r+

bors · Sep 11, 2017

📌 Commit d4e0e52 has been approved by petrochenkov

bors · Sep 12, 2017

⌛️ Testing commit d4e0e52 with merge 11f64d8...

bors · Sep 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 11f64d8 to master...

chris-morgan · Sep 20, 2017

It is worth noting that most syntax highlighters will need updating to support this. (I just did Vim.)

We need something like a mailing list for syntax highlighters where syntax changes can be announced.

Regular expression highlighters will now need something like \\u\{(?:\x_*){1,6}\}.

rust-highfive assigned nikomatsakis Aug 7, 2017

MaloJaffre force-pushed the MaloJaffre:_-in-literals branch from 47416d3 to 7d1fa02 Aug 7, 2017

arielb1 assigned petrochenkov and unassigned nikomatsakis Aug 8, 2017

arielb1 added the S-waiting-on-review label Aug 8, 2017

MaloJaffre force-pushed the MaloJaffre:_-in-literals branch from 14085a8 to 0bac86c Aug 13, 2017

petrochenkov added S-waiting-on-author S-waiting-on-team T-lang and removed S-waiting-on-review labels Aug 17, 2017

Accept underscores in unicode escapes
Fixes #43692.

Loading status checks…

d4e0e52

MaloJaffre force-pushed the MaloJaffre:_-in-literals branch from 0bac86c to d4e0e52 Aug 17, 2017

petrochenkov removed the S-waiting-on-author label Aug 20, 2017

rfcbot added the proposed-final-comment-period label Aug 25, 2017

rfcbot added final-comment-period and removed proposed-final-comment-period labels Sep 1, 2017

bors added a commit that referenced this pull request Sep 12, 2017

Auto merge of #43716 - MaloJaffre:_-in-literals, r=petrochenkov
Accept underscores in unicode escapes Fixes #43692. I don't know if this need an RFC, but at least the impl is here!

Loading status checks…

11f64d8

bors merged commit d4e0e52 into rust-lang:master Sep 12, 2017
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

homu Test successful
Details

MaloJaffre deleted the MaloJaffre:_-in-literals branch Sep 12, 2017

chris-morgan added a commit to rust-lang/rust.vim that referenced this pull request Sep 20, 2017

Support underscores in Unicode escapes
Rust gained this in rust-lang/rust#43716.

32d5688

ehuss referenced this pull request Sep 25, 2017
Merged
Add grammar for char, string, byte, and raw literals #121

dtolnay referenced this pull request Dec 25, 2017
Closed
Fails to lex underscore in unicode escapes #37

rust-lang/rust

Join GitHub today

Accept underscores in unicode escapes #43716

Conversation

MaloJaffre commented Aug 7, 2017

rust-highfive assigned nikomatsakis Aug 7, 2017

This comment has been minimized.

rust-highfive commented Aug 7, 2017

MaloJaffre force-pushed the MaloJaffre:_-in-literals branch from 47416d3 to 7d1fa02 Aug 7, 2017

kennytm reviewed Aug 7, 2017 View changes

This comment has been minimized.

This comment has been minimized.

kennytm commented Aug 7, 2017

This comment has been minimized.

arielb1 commented Aug 8, 2017

arielb1 assigned petrochenkov and unassigned nikomatsakis Aug 8, 2017

arielb1 added the S-waiting-on-review label Aug 8, 2017

This comment has been minimized.

petrochenkov commented Aug 9, 2017 • edited

This comment has been minimized.

SimonSapin commented Aug 9, 2017

This comment has been minimized.

joshtriplett commented Aug 9, 2017

This comment has been minimized.

petrochenkov commented Aug 12, 2017

MaloJaffre force-pushed the MaloJaffre:_-in-literals branch from 14085a8 to 0bac86c Aug 13, 2017

This comment has been minimized.

MaloJaffre commented Aug 13, 2017 • edited

petrochenkov reviewed Aug 17, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Aug 17, 2017 View changes

This comment has been minimized.

petrochenkov reviewed Aug 17, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MaloJaffre Aug 17, 2017 • edited

petrochenkov added S-waiting-on-author S-waiting-on-team T-lang and removed S-waiting-on-review labels Aug 17, 2017

This comment has been minimized.

petrochenkov commented Aug 17, 2017

This comment has been minimized.

petrochenkov commented Aug 17, 2017

MaloJaffre force-pushed the MaloJaffre:_-in-literals branch from 0bac86c to d4e0e52 Aug 17, 2017

This comment has been minimized.

MaloJaffre commented Aug 17, 2017 • edited

petrochenkov approved these changes Aug 17, 2017 View changes

petrochenkov removed the S-waiting-on-author label Aug 20, 2017

This comment has been minimized.

MaloJaffre commented Aug 25, 2017 • edited

This comment has been minimized.

petrochenkov commented Aug 25, 2017 • edited

This comment has been minimized.

SimonSapin commented Aug 25, 2017 • edited

This comment has been minimized.

aturon commented Aug 25, 2017

This comment has been minimized.

rfcbot commented Aug 25, 2017 • edited by withoutboats

rfcbot added the proposed-final-comment-period label Aug 25, 2017

pnkfelix reviewed Aug 30, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rfcbot commented Sep 1, 2017

rfcbot added final-comment-period and removed proposed-final-comment-period labels Sep 1, 2017

This comment has been minimized.

rfcbot commented Sep 11, 2017

This comment has been minimized.

petrochenkov commented Sep 11, 2017

This comment has been minimized.

bors commented Sep 11, 2017

This comment has been minimized.

bors commented Sep 12, 2017

bors added a commit that referenced this pull request Sep 12, 2017

This comment has been minimized.

bors commented Sep 12, 2017

Hide details View details bors merged commit d4e0e52 into rust-lang:master Sep 12, 2017 1 of 2 checks passed

1 of 2 checks passed

MaloJaffre deleted the MaloJaffre:_-in-literals branch Sep 12, 2017

MaloJaffre force-pushed the MaloJaffre:_-in-literals branch from `47416d3` to `7d1fa02` Aug 7, 2017

kennytm reviewed Aug 7, 2017

View changes

petrochenkov commented Aug 9, 2017 •

edited

MaloJaffre force-pushed the MaloJaffre:_-in-literals branch from `14085a8` to `0bac86c` Aug 13, 2017

MaloJaffre commented Aug 13, 2017 •

edited

petrochenkov reviewed Aug 17, 2017

View changes

petrochenkov reviewed Aug 17, 2017

View changes

petrochenkov reviewed Aug 17, 2017

View changes

MaloJaffre Aug 17, 2017 •

edited

MaloJaffre force-pushed the MaloJaffre:_-in-literals branch from `0bac86c` to `d4e0e52` Aug 17, 2017

MaloJaffre commented Aug 17, 2017 •

edited

petrochenkov approved these changes Aug 17, 2017

View changes

MaloJaffre commented Aug 25, 2017 •

edited

petrochenkov commented Aug 25, 2017 •

edited

SimonSapin commented Aug 25, 2017 •

edited

rfcbot commented Aug 25, 2017 •

edited by withoutboats

pnkfelix reviewed Aug 30, 2017

View changes

bors merged commit `d4e0e52` into rust-lang:master Sep 12, 2017
1 of 2 checks passed

chris-morgan commented Sep 20, 2017 •

edited