Fix bug in shebang handling

[rcoh]

Shebang handling was too agressive in stripping out the first line in cases where it is actually not a shebang, but instead, valid rust (#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 (#71372, #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 #70528

[rust-highfive]

r? @eddyb

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

[rcoh]

cc @petrochenkov this also fixes your comments on the previous PR (only the first line is whitespace-stripped, uses the correct is_whitespace function, & addresses the #!\n[...] case)

[petrochenkov]

r? @petrochenkov

[estebank]

This is a hack in the best meaning of the word.

[spastorino]

We could also place these kind of tests in a specific directory and filter those out?

[petrochenkov]

The filename opt out looks good to me.
It seems better to keep tests in directories where they actually belong to.

[eddyb]

cc @Mark-Simulacrum

[petrochenkov]

The filename opt out looks good to me.
It seems better to keep tests in directories where they actually belong to.

[eddyb]

I believe an empty #! shouldn't be considered a valid shebang, e.g.:

#!
[foo]

should be considered equivalent to #![foo].

The important thing, IMO, is to not treat valid Rust syntax (starting with #!, so only inner attributes) as shebangs. Comments would be the only exception, although we should take the conservative route and error if /* or // are found on the shebang line, instead of picking a behavior.

Having just wrangled C preprocessing (which has a similar problem with comment starting inside the header name of #include <...> directives, being implementation-defined), I'm now more than ever of the belief that when two syntaxes clash (here, shebangs and Rust), we should only allow disjoint cases, where only one possible interpretation is valid, and error on any overlaps.

[eddyb]

This PR should probably also add missing UI tests for #71372.

[rcoh]

On Monday, I will simplify & add UI tests.

[petrochenkov]

Ok, I guess we need to precisely define what is considered a shebang then (regardless of ambiguities with Rust).

No spaces, then #, then !, then what?

[rcoh]

After a surprisingly large amount of thought for this seemingly simple situation, I believe there is actually no solution that both accepts all valid rust programs and all valid shebangs other than attempting to parse the file and falling back to deleting the shebang line if it doesn't parse 🙄 (which I don't think we should do).

After a quick search, I don't think we'd break people if we were slightly stricter in how we handle shebangs, but I think that is the biggest risk with changing anything here. Most of these weird cases don't compile as is on stable so we won't break anyone by continuing to not have them compile.

My proposed change:
Shebangs are lines that:

1. Have at least 3 non whitespace characters
2. Begin with #!
3. Have a third character (after stripping whitespace) that isn't [

Notably this doesn't handle the case with //. I think that's OK.

Justification

My initial approach is defining shebangs as equivalent to the following regex: #![^\[\s]. But this suffers from this case:

#!// tricked you I'm a comment
[attribute(...)]

(which really looks like a shebang until you get to the 4th character)

But in terms of actual shebang acceptance,
#!//bin/bash is a totally valid shebang (and works fine on my laptop). This is sort of a problem because the first line can be both a shebang but also part of a valid program 😂

#!//bin/bash
[foo]

But just as well this could be

#!//bin/bash
pub fn not_a_sneaky_attribute() -> {
  println!("Because everything sucks here, that was actually a shebang. Good luck figuring that out.");
}

It's worth noting this doesn't currently compile: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=a0603ad05e578e7eaf118d540e8bcd49 (but I think it should)

And also, #! /bin/bash is a valid shebang according https://unix.stackexchange.com/questions/276751/is-space-allowed-between-and-bin-bash-in-shebang/276753 (so a space after shebang is actually allowed).

And regardless of theoretical validity, #! /bin/bash works fine as a shebang.

But, #! and nothing else is invalid, which I think is what we need to check for.

[eddyb]

Note that a Rust module cannot start with [, so #! followed by whitespace/comments followed by [ could not be a shenang followed by Rust module.

My suggestion was to disallow any potential overlap and error if // or /* are found in the shebang. We could also ban \, $ or !, which could have meaning in the future, and maybe '/" because of char/string literals. But it's perhaps getting into the weeds.

IMO we should do this properly. I'll prepare a PR reverting the previous PR so that we can crater-run this one (EDIT: it's up as #71634).

[petrochenkov]

I believe there is actually no solution that both accepts all valid rust programs and all valid shebangs other than attempting to parse the file and falling back to deleting the shebang line if it doesn't parse 🙄 (which I don't think we should do).

We only need lexing, not parsing, and we only need to run the lexer loop until we encounter a first non-whitespace token. Seems pretty realistic :)

This will also allow to avoid processing things like // or /* manually. (Comments are whitespace tokens.)

Pseudocode:

fn is_shebang = first_line.starts_with("#!") && first_line[2..].contains(non_whitespace_character)

fn is_rust = non_whitespace_tokens.starts_with(#, !, [)

If it's only is_shebang or only is_rust, then the choice is clear.
It it's both, choose rust or produce an error.

[bors]

📌 Commit f1e14736e785d0353b7dc9f1174499ef94ae78d7 has been approved by petrochenkov

[bors]

⌛ Testing commit f1e14736e785d0353b7dc9f1174499ef94ae78d7 with merge d62fa51f12dfec9bba0b7c6cbf66fd1dee52062c...

[rcoh]

Looks like someone else fixed the same #FIXME

error[E0201]: duplicate definitions with name `pos`:
   --> src/librustc_parse/lexer/mod.rs:423:5
    |
74  | /     pub fn pos(&self) -> BytePos {
75  | |         self.pos
76  | |     }
    | |_____- previous definition of `pos` here
...
423 | /     pub fn pos(&self) -> BytePos {
424 | |         self.pos
425 | |     }
    | |_____^ duplicate definition

error: aborting due to previous error

For more information about this error, try `rustc --explain E0201`.
error: could not compile `rustc_parse`.

[petrochenkov]

@bors r+

[bors]

📌 Commit a93d316 has been approved by petrochenkov

[bors]

⌛ Testing commit a93d316 with merge b0fa18865969ed43e27c1fb8fcc1acb2c0aa60e0...

[Dylan-DPC-zz]

@bors retry yield

[bors]

⌛ Testing commit a93d316 with merge 9eedd13...

[bors]

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 9eedd13 to master...