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

scanner: disallow shebang outside of *.vsh files #21029

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Mar 15, 2024

No description provided.

@spytheman
Copy link
Member

why?

@ttytm
Copy link
Member Author

ttytm commented Mar 15, 2024

Good question. Could be better to keep it valid it for .v files and treat it as a shebang is in script files. Recognized as shebang when it's at the beginning of the file.

Currently this compiles. Which got me into this PR.

module main

type Bar = string

fn main() {
	#!/usr/bin/env -S v
	dump('abc')
}

@ttytm
Copy link
Member Author

ttytm commented Mar 15, 2024

In case #! shouldn't be a valid way for regular comments.

@spytheman
Copy link
Member

I think it should be valid, only on the first line, no matter the extension.

@spytheman
Copy link
Member

spytheman commented Mar 15, 2024

Note that you can:

#!/usr/bin/env -S v -raw-vsh-tmp-prefix tmp run
dump('abc')

Save this file as just abc (without any extension), then do chmod 755 abc, and then do: ./abc

@spytheman
Copy link
Member

spytheman commented Mar 15, 2024

You can also skip the run in the first line, in which case, v will generate a tmp.abc executable just once, and doing ./abc after that, will just start ./tmp.abc directly, without having to compile it again, which will be much faster.
That is a documented feature, see https://github.com/vlang/v/blob/master/doc/docs.md#vsh-scripts-with-no-extension, and making the scanner depend on the extension will break it.

#0 10:40:00 ᛋ master /v/oo❱touch abc
#0 10:40:04 ᛋ master /v/oo❱xtime ./abc
[./abc:2] 'abc': abc
CPU: 0.33s      Real: 0.31s     Elapsed: 0:00.31        RAM: 58268KB    ./abc
#0 10:40:05 ᛋ master /v/oo❱xtime ./abc
[./abc:2] 'abc': abc
CPU: 0.00s      Real: 0.00s     Elapsed: 0:00.00        RAM: 5064KB     ./abc
#0 10:40:06 ᛋ master /v/oo❱xtime ./abc
[./abc:2] 'abc': abc
CPU: 0.00s      Real: 0.00s     Elapsed: 0:00.00        RAM: 5064KB     ./abc
#0 10:40:07 ᛋ master /v/oo❱

@ttytm
Copy link
Member Author

ttytm commented Mar 15, 2024

With the new changes a shebang that's not on the first line is disallowed.

Shebangs are not recognized when it has empty lines beforehand.

script.sh: won't use cat if there's an empty line beforehand:


#!/bin/cat

echo 'hello'

script.vsh: with an empty line


#!/usr/bin/env -S v

println('hello world')

Currently results in:

./test.vsh: line 4: syntax error near unexpected token `'hello world''
./test.vsh: line 4: `println('hello world')'

With the changes it would be:

bugs/shebang.v:3:1: error: a shebang is only valid at the top of the file
    1 | 
    2 | 
    3 | #!/usr/bin/env -S v
      | ^
    4 | 
    5 | println('hello')

The new error will stop v fmt to remove the empty lines beforehand

Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

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

Excellent work.

@spytheman spytheman merged commit c024721 into vlang:master Mar 15, 2024
54 checks passed
@ttytm ttytm deleted the scanner/shebang-only-for-vsh-files branch March 15, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants