Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error message for using >= 65535 hashes for raw string literal escapes #50296

Merged
merged 1 commit into from
Jun 15, 2018

Conversation

dcao
Copy link

@dcao dcao commented Apr 28, 2018

Fixes #50111.

@kennytm kennytm added T-lang Relevant to the language team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2018
@kennytm
Copy link
Member

kennytm commented Apr 28, 2018

r? @eddyb or @varkor

@rust-highfive

This comment has been minimized.

// except according to those terms.

fn main() {
let x = r################################################################################################################################################################################################################################################################"test"################################################################################################################################################################################################################################################################
Copy link
Member

Choose a reason for hiding this comment

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

This file needs an // ignore-tidy-linelength header.

@rust-highfive

This comment has been minimized.

Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

It's looking good to me! I just left a few very minor comments.

The test is currently failing because the offset in the line/column number is wrong (after you added the line to ignore tidy), but once you tweak the range/text you can just regenerate it.

error: too many `#` characters surrounding a raw string; only 255 or fewer characters can be used
--> src/test/ui/raw_string_hash_count.rs:12:13
|
LL | let x = r################################################################################################################################################################################################################################################################"test"################################################################################################################################################################################################################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to offset the range by 1, so that it underlines all the # on the left hand side (i.e. at the moment, the r is included, but not the final #).

// ignore-tidy-linelength

fn main() {
let x = r################################################################################################################################################################################################################################################################"test"################################################################################################################################################################################################################################################################
Copy link
Member

Choose a reason for hiding this comment

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

For simplicity, you can eliminate the variable as well, and just have the raw string as a standalone statement.

let last_bpos = self.pos;
self.fatal_span_(start_bpos,
last_bpos,
"too many `#` characters surrounding a raw string; \
Copy link
Member

Choose a reason for hiding this comment

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

Maybe clarify to "raw byte string". It's probably fine to only test one of them, though, as it's a minor error. Also, the indentation here is off.

let last_bpos = self.pos;
self.fatal_span_(start_bpos,
last_bpos,
"too many `#` characters surrounding a raw string; \
Copy link
Member

Choose a reason for hiding this comment

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

The wording could be a little more concise here. Maybe:

too many `#` symbols: raw strings may be delimited by up to 255 `#` symbols

(and same for byte strings).

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@@ -0,0 +1,8 @@
error: too many `#` symbols: raw strings may be delimited by up to 255 `#` symbols
--> src/test/ui/raw_string_hash_count.rs:14:5
Copy link
Member

Choose a reason for hiding this comment

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

You need to replace src/test/ui with $DIR.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:44:42] ....................................................................................................
[00:44:49] ....................................................................................................
[00:44:55] ....................................................................................................
[00:45:03] ....................................i...............................................................
[00:45:09] ............i.........................................F.............................................
[00:45:23] ....................................................................................................
[00:45:30] ...........i....................................................................
[00:45:30] failures:
[00:45:30] 
[00:45:30] 
[00:45:30] ---- [ui] ui/raw_string_hash_count.rs stdout ----
[00:45:30]  
[00:45:30] error: /checkout/src/test/ui/raw_string_hash_count.rs:14: unexpected error: '14:5: 14:262: too many `#` symbols: raw strings may be delimited by up to 255 `#` symbols'
[00:45:30] 
[00:45:30] error: /checkout/src/test/ui/raw_string_hash_count.rs:14: expected error not found: too many `#` characters
[00:45:30] 
[00:45:30] error: 1 unexpected errors found, 1 expected errors not found
[00:45:30] status: exit code: 101
[00:45:30] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/raw_string_hash_count.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/raw_string_hash_count.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/raw_string_hash_count.stage2-x86_64-unknown-linux-gnu.aux" "-A" "unused"
[00:45:30] unexpected errors (from JSON output): [
[00:45:30]     Error {
[00:45:30]         line_num: 14,
[00:45:30]         kind: Some(
[00:45:30]             Error
[00:45:30]         ),
[00:45:30]         msg: "14:5: 14:262: too many `#` symbols: raw strings may be delimited by up to 255 `#` symbols"
[00:45:30] ]
[00:45:30] 
[00:45:30] not found errors (from test file): [
[00:45:30]     Error {
[00:45:30]     Error {
[00:45:30]         line_num: 14,
[00:45:30]         kind: Some(
[00:45:30]             Error
[00:45:30]         ),
[00:45:30]         msg: "too many `#` characters"
[00:45:30] ]
[00:45:30] 
[00:45:30] thread '[ui] ui/raw_string_hash_count.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1278:13
[00:45:30] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:45:30] 
[00:45:30] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:488:22
[00:45:30] 
[00:45:30] 
[00:45:30] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:45:30] 
[00:45:30] 
[00:45:30] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:45:30] Build completed unsuccessfully in 0:02:45
[00:45:30] Build completed unsuccessfully in 0:02:45
[00:45:30] Makefile:58: recipe for target 'check' failed
[00:45:30] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:044c0ffa
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)


fn main() {
r################################################################################################################################################################################################################################################################"test"################################################################################################################################################################################################################################################################;
//~^ ERROR too many `#` characters
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be updated to use symbols instead of characters.

@eddyb
Copy link
Member

eddyb commented Apr 30, 2018

Why do we want to do this? cc @rust-lang/compiler

@nagisa
Copy link
Member

nagisa commented Apr 30, 2018

Probably for better diagnostics? We either introduce some limit or make the number of hashes technically unlimited by counting them into u64 or larger.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 30, 2018

Idk. Doesn't feel like something that we are allowed to break under the breaking change rules. Then again, seems like an accident that such riddiculous numbers of hashes are allowed.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 30, 2018

I feel ok about going to u8 for now if it'll help with perf. Searching through crates.io with a simple regex, it seems pretty clear that this will not break anything.

@eddyb
Copy link
Member

eddyb commented Apr 30, 2018

@nikomatsakis I don't thinks this helps anything. @nnethercote changed it to u16 for a size win, but going to u8 doesn't reduce that size any further.

@nikomatsakis
Copy link
Contributor

OK, point is, we can impose a limit. I'm indifferent about what limit we impose.

@estebank
Copy link
Contributor

estebank commented May 1, 2018

@CMDD could you squash your commits?

@dcao
Copy link
Author

dcao commented May 1, 2018

@estebank commits have been squashed, hopefully without messing anything up in the process

@eddyb
Copy link
Member

eddyb commented May 1, 2018

I'm nominating this PR for discussion, on the topic of what limits can an implementation reasonably have (rustc currently uses u32 in a bunch of places where usize would be impossible to overflow).

@shepmaster shepmaster added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2018
@joshtriplett
Copy link
Member

We discussed this in the lang team meeting, and we all agreed that we can safely limit this to 255 without concern.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 10, 2018
@nikomatsakis
Copy link
Contributor

@CMDD merge conflict :(

@pietroalbini
Copy link
Member

@CMDD you should rebase on master, even if you don't see any conflict at first glance.

@dcao
Copy link
Author

dcao commented Jun 8, 2018

Sorry for the delay! I've rebased onto master.

@dcao
Copy link
Author

dcao commented Jun 13, 2018

@nikomatsakis Are these changes good to be pushed onto rust-lang/master?

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 15, 2018

📌 Commit 313d6c5 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 15, 2018
@bors
Copy link
Contributor

bors commented Jun 15, 2018

⌛ Testing commit 313d6c5 with merge ab0a442...

bors added a commit that referenced this pull request Jun 15, 2018
Add error message for using >= 65535 hashes for raw string literal escapes

Fixes #50111.
@bors
Copy link
Contributor

bors commented Jun 15, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 15, 2018
@pietroalbini
Copy link
Member

@bors retry #51565

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 15, 2018
@bors
Copy link
Contributor

bors commented Jun 15, 2018

⌛ Testing commit 313d6c5 with merge 2bd50a523053b5bd02014a73e45a072663efcb53...

@pietroalbini
Copy link
Member

Give priority to #51565, which should fix the failure.

@bors retry

@bors
Copy link
Contributor

bors commented Jun 15, 2018

⌛ Testing commit 313d6c5 with merge 967c1f3...

bors added a commit that referenced this pull request Jun 15, 2018
Add error message for using >= 65535 hashes for raw string literal escapes

Fixes #50111.
@bors
Copy link
Contributor

bors commented Jun 15, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 967c1f3 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.