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

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

Closed
eddyb opened this issue Mar 29, 2020 · 6 comments · Fixed by #71487
Closed

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

eddyb opened this issue Mar 29, 2020 · 6 comments · Fixed by #71487
Assignees
Labels
A-parser Area: The parsing of Rust source code to an AST. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Mar 29, 2020

A file containing only this correctly errors:

#![bad_attribute]

But this doesn't error, as it's stripped the same way #!/usr/bin/env ... (i.e. "shebang") would be:

#! [bad_attribute]

In this third example, it also strips just the #!, breaking the attribute:

#!
[bad_attribute]

The code responsible is this:

/// Line won't be skipped if it represents a valid Rust syntax
/// (e.g. "#![deny(missing_docs)]").
pub fn strip_shebang(input: &str) -> Option<usize> {
debug_assert!(!input.is_empty());
if !input.starts_with("#!") || input.starts_with("#![") {

It doesn't seem to account for any whitespace between the ! and [.
I believe that we should allow any characters c where c != '\n' && is_whitespace(c) after the #!, and determine whether this is a shebang, by the next character on the same line:

  • if there are no non-whitespace characters, it's not a valid shebang
  • if the next character is [, this is the start of an inner comment
  • otherwise, it's probably a shebang

I have no idea what to do for Rust comment syntax (#!//... and #!/*...), however.

And, of course, at the end of the day, this might be a backwards-incompatible change to make.

cc @matklad @petrochenkov

This issue has been assigned to @rcoh via this comment.

@eddyb eddyb added A-parser Area: The parsing of Rust source code to an AST. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Mar 29, 2020
@ayushmishra2005
Copy link
Contributor

@eddyb

As I understood from the description, you are saying that

  1. This errors should also come for #! [bad_attribute].

  2. #! /usr/bin/end should also be considered as valid shebang and Rust compiler will skip this line.

  3. no changes for the third scenario:
    #!
    [bad_attribute]

Please confirm if we are on same page.

@eddyb
Copy link
Member Author

eddyb commented Apr 18, 2020

@ayushmishra2005 All of those sound correct, yeah

@ayushmishra2005
Copy link
Contributor

Thanks @eddyb . I would like to pick this issue.

@rustbot claim

@rustbot rustbot self-assigned this Apr 19, 2020
ayushmishra2005 pushed a commit to ayushmishra2005/rust that referenced this issue Apr 21, 2020
ayushmishra2005 pushed a commit to ayushmishra2005/rust that referenced this issue Apr 21, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this issue Apr 22, 2020
…r=estebank

Fix #! (shebang) stripping account space issue

 rust-lang#70528
@JohnTitor
Copy link
Member

Fixed by #71372.

@eddyb
Copy link
Member Author

eddyb commented Apr 28, 2020

There are issues with the fix (and it didn't go through Crater), so I'm preparing a revert.

@rcoh
Copy link
Contributor

rcoh commented May 9, 2020

@rustbot claim

@rustbot rustbot assigned rustbot and unassigned rustbot May 9, 2020
rcoh added a commit to rcoh/rust that referenced this issue May 25, 2020
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.
@bors bors closed this as completed in 9eedd13 May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parser Area: The parsing of Rust source code to an AST. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants