Fix #! (shebang) stripping account space issue#71372

ayushmishra2005 · 2020-04-21T06:17:27Z

#70528

rust-highfive · 2020-04-21T06:17:31Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

src/librustc_lexer/src/lib.rs

JohnTitor · 2020-04-21T06:31:15Z

Also, it'd be great that if you could also include tests.

src/librustc_lexer/src/lib.rs

estebank · 2020-04-21T22:37:36Z

@bors r+ rollup

bors · 2020-04-21T22:37:37Z

📌 Commit 1b362cd has been approved by estebank

@ghost

Rollup of 7 pull requests Successful merges: - rust-lang#70998 (Suggest `-> impl Trait` and `-> Box<dyn Trait>` on fn that doesn't return) - rust-lang#71236 (Remove unused rustc_serialize::hex module) - rust-lang#71366 (Use assoc int consts3) - rust-lang#71372 (Fix #! (shebang) stripping account space issue) - rust-lang#71384 (Fix stage0.txt version number comment) - rust-lang#71390 (Fix incorrect description of E0690) - rust-lang#71399 (Clean up E0554 explanation) Failed merges: r? @ghost

petrochenkov · 2020-04-22T17:24:57Z

src/librustc_lexer/src/lib.rs

 }

+fn remove_whitespace(s: &str) -> String {
+    s.chars().filter(|c| !c.is_whitespace()).collect()


This should use is_whitespace from rustc_lexer instead of the one from libstd.

It also doesn't seem reasonable to filter and re-collect all the program text to check for something that almost never happens, or requires checking a couple of characters when it does.

Looks like in this case

#! [bad_attribute]

#! is no longer treated as a shebang anymore, which also seems incorrect.

I think an empty shebang shouldn't be treated as a shebang, especially as it can be part of valid Rust syntax.

But I agree with everything else, and I wish this PR had been assigned to you...

Just found #71487, will leave comments there.

eddyb · 2020-04-27T01:27:47Z

src/librustc_lexer/src/tests.rs

+    #[test]
+    fn test_valid_shebang() {
+        // https://github.com/rust-lang/rust/issues/70528
+        let input = "#!/usr/bin/rustrun";
+        let actual = strip_shebang(input);
+        let expected: Option<usize> = Some(18);
+        assert_eq!(expected, actual);
+    }
+
+    #[test]
+    fn test_invalid_shebang_valid_rust_syntax() {
+        // https://github.com/rust-lang/rust/issues/70528
+        let input = "#!    [bad_attribute]";
+        let actual = strip_shebang(input);
+        let expected: Option<usize> = None;
+        assert_eq!(expected, actual);
+    }


There should be UI tests as well.

…ipping, r=estebank" This reverts commit 46a8dce, reversing changes made to f28e387.

@petrochenkov

Revert rust-lang#71372 ("Fix #! (shebang) stripping account space issue"). While rust-lang#71372 fixed some of the problems `#!`-stripping had, it introduced others: * inefficient implementation (`.chars().filter(...).collect()` on the entire input file) * this also means the length returned isn't always correct, leading to e.g. rust-lang#71471 * it ignores whitespace anywhere, stripping ` # ! ...` which isn't a valid shebang * the definition of "whitespace" it uses includes newlines, which means even `\n#\n!\n...` is stripped as a shebang (and anything matching the regex `\s*#\s*!\s*`, and not followed by `[`, really) * it's backward-incompatible but didn't go through Crater Now, rust-lang#71487 is already open and will solve all of these issues. But for running Crater, and just in case rust-lang#71487 takes a bit longer, I decided it's safer to just revert rust-lang#71372. This will also make rust-lang#71372's diff clearer, as it will start again from the original whitespace-unaware version. r? @petrochenkov

@ghost

Rollup of 5 pull requests Successful merges: - rust-lang#71311 (On `FnDef` type annotation suggestion, use fn-pointer output) - rust-lang#71488 (normalize field projection ty to fix broken MIR issue) - rust-lang#71489 (Fix off by one in treat err as bug) - rust-lang#71585 (remove obsolete comment) - rust-lang#71634 (Revert rust-lang#71372 ("Fix #! (shebang) stripping account space issue").) Failed merges: r? @ghost

Shebang handling was too agressive in stripping out the first line in cases where it is actually _not_ a shebang, but instead, valid rust (rust-lang#70528). This is a second attempt at resolving this issue (the first attempt was flawed, for, among other reasons, causing an ICE in certain cases (rust-lang#71372, rust-lang#71471). The behavior is now codified by a number of UI tests, but simply: For the first line to be a shebang, the following must all be true: 1. The line must start with `#!` 2. The line must contain a non whitespace character after `#!` 3. The next character in the file, ignoring comments & whitespace must not be `[` I believe this is a strict superset of what we used to allow, so perhaps a crater run is unnecessary, but probably not a terrible idea.

Fix bug in shebang handling Shebang handling was too agressive in stripping out the first line in cases where it is actually _not_ a shebang, but instead, valid rust (rust-lang#70528). This is a second attempt at resolving this issue (the first attempt was reverted, for, among other reasons, causing an ICE in certain cases (rust-lang#71372, rust-lang#71471). The behavior is now codified by a number of UI tests, but simply: For the first line to be a shebang, the following must all be true: 1. The line must start with `#!` 2. The line must contain a non-whitespace character after `#!` 3. The next character in the file, ignoring comments & whitespace must not be `[` I believe this is a strict superset of what we used to allow, so perhaps a crater run is unnecessary, but probably not a terrible idea. Fixes rust-lang#70528

Fix #! (shebang) stripping account space issue rust-lang#70528

0315864

rust-highfive assigned estebank Apr 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2020

JohnTitor reviewed Apr 21, 2020

View reviewed changes

src/librustc_lexer/src/lib.rs Outdated Show resolved Hide resolved

petrochenkov self-assigned this Apr 21, 2020

Dylan-DPC-zz unassigned estebank Apr 21, 2020

Refactoring and added test-cases rust-lang#70528

ee5a212

ayushmishra2005 requested a review from JohnTitor April 21, 2020 11:20

Fix formatting issue

365b3cc

estebank reviewed Apr 21, 2020

View reviewed changes

src/librustc_lexer/src/lib.rs Outdated Show resolved Hide resolved

Minor refactoring

1b362cd

ayushmishra2005 requested a review from estebank April 21, 2020 17:01

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 Apr 21, 2020

JohnTitor mentioned this pull request Apr 22, 2020

Rollup of 7 pull requests #71410

Merged

bors merged commit 46a8dce into rust-lang:master Apr 22, 2020

JohnTitor mentioned this pull request Apr 22, 2020

#! (shebang) stripping doesn't account for spaces. #70528

Closed

petrochenkov reviewed Apr 22, 2020

View reviewed changes

eddyb reviewed Apr 27, 2020

View reviewed changes

eddyb mentioned this pull request Apr 27, 2020

Fix bug in shebang handling #71487

Merged

eddyb added a commit to eddyb/rust that referenced this pull request Apr 28, 2020

Revert "Rollup merge of rust-lang#71372 - ayushmishra2005:shebang_str…

4d67c8d

…ipping, r=estebank" This reverts commit 46a8dce, reversing changes made to f28e387.

eddyb mentioned this pull request Apr 28, 2020

Revert #71372 ("Fix #! (shebang) stripping account space issue"). #71634

Merged

Dylan-DPC-zz mentioned this pull request Apr 28, 2020

Rollup of 5 pull requests #71636

Merged

Alexendoo mentioned this pull request Apr 29, 2020

ices/71471.rs: fixed with errors rust-lang/glacier#345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix #! (shebang) stripping account space issue#71372