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

Interpreter allows arbitrary commands to be executed via the comment #1504

Closed
harendra-kumar opened this Issue Dec 12, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@harendra-kumar
Collaborator

harendra-kumar commented Dec 12, 2015

For example:

cueball$ cat test.hs 
#!/usr/bin/env stack
-- stack exec ghci

main = putStrLn "Hello!"

cueball$ stack test.hs
GHCi, version 7.10.2: http://www.haskell.org/ghc/  :? for help
[1 of 1] Compiling Main             ( test.hs, interpreted )
Ok, modules loaded: Main.
*Main> 

It can confuse and surprise if one writes a wrong comment.

I have a fix which restricts the commands to runhaskell and runghc in the interpreter mode.

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Dec 12, 2015

Though I think it might be useful to even allow the ghci command as in the example output above, to automatically load a script in ghci every time you run it. But other commands may not make sense.

@kadoban

This comment has been minimized.

Collaborator

kadoban commented Dec 12, 2015

So here's a question I've been thinking about, which seems related to this:
What's even the point of allowing/requiring the stack and runghc parts on the second line of

#!/usr/bin/env stack
-- stack runghc --install-ghc

or whatever?

I assume the reasoning it's required in the case where options actually need to be specified is to make it less ambiguous if a comment at the start is for stacks consumption or not, but wouldn't some kind of pragma syntax be preferable to that, like:

#!/usr/bin/env stack
{-# STACK --install-ghc #-}

That would make it unambiguous, it'd fit in with other pragmas better, and it'd mean that that extra line wouldn't be required when nothing needs to be specified.

I ask because ... coming from using shebangs in other languages like python, I don't know of any of them that require an extra configuration line like that, so it stands out as really weird to me.

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Dec 12, 2015

This is really a hack for stack and not required by language per se. If not using stack the shebang would be a standard /usr/bin/env runhaskell.

In the current form it is a real valid stack command which can be run on the command line with the file name added at the end. A pragma like syntax is trivial to support but the real question is what should be the content of the pragma syntax? Will it be a valid stack command? If not then in what way will it be different than a command?

We can omit the command and assume it to be runghc or runhaskell and let all other runghc options be specified in the pragma? But then the question is whether an explicit command exactly equivalent to how you will use it on the stack command line is better (less confusing) than an implicit command?

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Dec 12, 2015

I agree the current syntax is a bit weird and non standard. A pragma like syntax is already known and learnt and looks better to me.

Also see #1394 where a pragma like syntax was suggested earlier and was countered by @borsboom .

Another point is that since it is not a ghc recognised pragma, we will have to pass something like -fno-warn-unrecognised-pragmas to ghc so that it does not emit a warning while compiling this file.

@kadoban

This comment has been minimized.

Collaborator

kadoban commented Dec 13, 2015

I'd worry about that being confused with compiler pragmas, and people expecting to to be used in all cases (i.e. by stack build as well). Since this only applies when using stack as a script interpreter, that would be misleading.

I'm not really clear on how a pragma would be more confusing in that regard than the current method. If anything it could probably be worded to be less confusing with the right "word" in {-# word --whatever-options #-}

As to GHC being noisy about it, maybe the GHC people could be argued into not warning about a pragma that doesn't make sense for them to handle and obviously isn't a typo of one they should.

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Dec 13, 2015

Let me try a concrete proposal so that it is easier to debate and request concrete feedback from others. Let's consider replacing the current syntax with a pragma as follows:

{-# RUNSTACK --resolver lts-3.16 --install-ghc --package turtle #-}

alternately when the line gets too long:

{-# RUNSTACK 
     --resolver lts-3.16 
     --install-ghc 
     --package turtle 
#-}

Now what would be valid content inside the pragma? To keep it consistent and simpler to understand, we can have a rule that if you replace RUNSTACK with stack runghc the pragma should form an equivalent stack command line.

So the equivalent command line corresponding to the pragma above would be:
stack runghc --resolver lts-3.16 --install-ghc --package turtle <file path>

The keyword RUNSTACK indicates that this is a pragma for running the file using stack and the naming is consistent with other ways to run a script like runghc and runhaskell. The prefix RUN would avoid confusing it with GHC options or stack build options, the point raised earlier by Emanuel.

Requesting feedback on the above from people involved in the previous discussion about this i.e. @ndmitchell , @borsboom , @mgsloan .

harendra-kumar added a commit to harendra-kumar/stack that referenced this issue Dec 13, 2015

Restrict commands allowed in interpreter mode
Stack interpreter comment annotation allows arbitrary commands to be
written and executed when the file is run. It can lead to confusing
and surprising behavior if mistakes are made in writing a proper
comment.

This change restricts the interpreter mode commands to runghc and
runhaskell. This change moves add-commands to a separate function.

Closes commercialhaskell#1504
@kadoban

This comment has been minimized.

Collaborator

kadoban commented Dec 13, 2015

To me the proposal there looks nice. I'd be interested to see what the devs think about it.

(only comment I'd have is I'm not sure if the multi-line thing is allowed syntax for haskell pragmas, if not another option would be to allow multiple pragmas and just concatenate them all together)

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Dec 13, 2015

I checked it before proposing :-) It seems to pass through GHC. There is a corner case though, if the last line (i.e. #-}) in the example above is not preceded by a space then GHC complains. Though no space is necessary if the whole thing is on the same line. This just does not seem to be by design, perhaps nobody ever noticed or cared about it since most pragmas are single line.

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Dec 14, 2015

Hmm, I don't see any benefit to using pragma syntax. The only thing I can think of is that it's normal for {-# #-} to enclose comments that have some machine meaning. The effort required to make such a change + the fact that GHC warns about these seems like enough of a reason to not do it.

@harendra-kumar

This comment has been minimized.

Collaborator

harendra-kumar commented Dec 14, 2015

The argument in favor of it is consistency with the widely known and accepted pragma syntax. For example, just by looking at it someone not even knowing about the stack comment syntax can perhaps easily tell that it is some directive rather than a simple innocuous comment.

If we agree that this is the right thing, effort is not a problem. We already support the {- ... -} syntax we just need to add a # in the parser.

I have not investigated the GHC warning further but maybe it can be solved by passing an extra flag like -fno-warn-unrecognised-pragmas to runghc? A motivated soul can figure that out :-) But assuming that it can be solved cleanly, is the pragma syntax the right thing? Are there any pitfalls?

I like it only for the consistency argument. On the other hand, the good thing about the current syntax is that its a plain stack command which is a relatively simple fact to know. Also, it is more powerful, for example by using a exec ghci command there instead of runghc I can setup any file to be loaded in ghci when it is executed. Not sure if that's very useful though. Again, its all about the semantics and can be retained with the new syntax as well if we want - but its not in my proposal above because it assumes the command to be runghc.

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Dec 14, 2015

The argument in favor of it is consistency with the widely known and accepted pragma syntax. For example, just by looking at it someone not even knowing about the stack comment syntax can perhaps easily tell that it is some directive rather than a simple innocuous comment.

It doesn't seem consistent to me, because pragmas are for the compiler. To me, #!/usr/bin/env stack preceding the comment is enough of an indication that something special is happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment