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

REPL broken on main #58859

Open
weissi opened this issue May 12, 2022 · 27 comments
Open

REPL broken on main #58859

weissi opened this issue May 12, 2022 · 27 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. regression REPL

Comments

@weissi
Copy link
Member

weissi commented May 12, 2022

Describe the bug

On main, if you start the REPL by typing swift you don't actually get into the REPL, you'll just see the help and you're back at the shell prompt (instead of the REPL prompt).

To Reproduce

Steps to reproduce the behavior:

  1. docker pull swiftlang/swift:nightly-main2.
  2. docker run -it --rm --privileged swiftlang/swift:nightly-main swift3.

Expected behavior

A REPL prompt like in Swift 5.6 (and earlier):

$ docker run -it --rm --privileged swift:5.6 swift

[...]

Welcome to Swift version 5.6.1 (swift-5.6.1-RELEASE).
Type :help for assistance.
  1>  

Screenshots

Screenshot 2022-05-12 at 4 52 38 pm

Environment (please complete the following information):

$ docker run -it --rm --privileged swiftlang/swift:nightly-main swift -version
Swift version 5.7-dev (LLVM 0111970099b6de6, Swift b6ec1874c96449c)
Target: x86_64-unknown-linux-gnu
@weissi weissi added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. REPL regression labels May 12, 2022
@philipturner
Copy link
Contributor

REPL hasn't been working on Windows as far as I can remember. I think it's also been that way on Linux. In other words, it only has ever worked on macOS. If what I have concluded is correct, this wouldn't be a "regression" if it never did work correctly in the first place.

@weissi
Copy link
Member Author

weissi commented May 12, 2022

@philipturner it always worked on Linux up to including 5.6. It is definitely a regression.

@NSAntoine
Copy link
Contributor

NSAntoine commented May 12, 2022

I saw a commit somewhere in the driver that this was intentional, don’t think this is a bug or anything

@NSAntoine
Copy link
Contributor

@weissi
Copy link
Member Author

weissi commented May 13, 2022

apple/swift-driver@1cf2fd0

Oh thank you! That makes (some) sense (although quite the change in a minor release) and I guess my macOS toolchain is just not new enough to get that functionality.

@bitjammer I wasn't able to find the accepted Swift Evolution proposal for this change. Mind sharing that with me?

@bitjammer
Copy link
Member

Yes, this is expected behavior. swift repl is the new usage. There is not an SE for the command line change.

@bitjammer
Copy link
Member

However there was discussion about CL UX changes here: https://forums.swift.org/t/command-line-ux-enhancements-for-swift

@weissi
Copy link
Member Author

weissi commented May 13, 2022

Okay thanks. @tomerd is that always the case that command line UX changes do not go through Evo?

In this case, it may actually break scripts.

Previously, this would run code:

$ echo 'print("HELLO WORLD, THIS RUNS CODE.")' | docker run -i --rm --privileged swift:5.5 swift
Welcome to Swift version 5.5.3 (swift-5.5.3-RELEASE).
Type :help for assistance.
HELLO WORLD, THIS RUNS CODE.

now, it does no longer do that.

$ echo 'print("HELLO WORLD, THIS RUNS CODE.")' | docker run -i --rm --privileged swiftlang/swift:nightly-main swift

Welcome to Swift!

Subcommands:

  swift build      Build Swift packages
  swift package    Create and work on packages
  swift run        Run a program from a package
  swift test       Run package tests
  swift repl       Experiment with Swift code interactively

  Use `swift --help` for descriptions of available options and flags.

  Use `swift help <subcommand>` for more information about a subcommand.

@NSAntoine
Copy link
Contributor

Change | docker run -i --rm --privileged swiftlang/swift:nightly-main swift to | docker run -i --rm --privileged swiftlang/swift:nightly-main swift repl?

@weissi
Copy link
Member Author

weissi commented May 13, 2022

Change | docker run -i --rm --privileged swiftlang/swift:nightly-main swift to | docker run -i --rm --privileged swiftlang/swift:nightly-main swift repl?

Sure, I'm just saying that this is a (potentially) breaking change. So some scripts may be broken by this change. Just FTR: I'm not against this change but I used to use this to quickly test stuff in docker. Of course adding a repl argument isn't hard but if somebody scripted something, it'll break once released.

@NSAntoine
Copy link
Contributor

I guess what the developers of swift could do is detect for stdin input and then launch repl from there, which would maintain compatibility with these scripts, wouldn’t it?

@tomerd
Copy link
Member

tomerd commented May 13, 2022

@bitjammer, @weissi makes a good point about this change breaking scripts / existing flows. could we detect the mode of trying to run a script (eg by examining the inputs) and "do the right thing"?

@NSAntoine
Copy link
Contributor

NSAntoine commented May 13, 2022

@bitjammer, @weissi makes a good point about this change breaking scripts / existing flows. could we detect the mode of trying to run a script (eg by examining the inputs) and "do the right thing"?

What you can do is detect if swift is being piped into:

let isBeingPiped = (isatty(STDIN_FILENO) == 0)
if isBeingPiped {
// trigger REPL
}

I actually tried changing the code to do that, and it did actually detect when its being piped into (ie, echo 'print("hi!")' | swift), however for whatever reason REPL immiedately starts & dies when triggering it..

@bitjammer
Copy link
Member

bitjammer commented May 13, 2022

What I was trying to move toward with this change was making the REPL an equal peer to other subcommands so that a more generalized, context-aware swift tool could develop. Detecting stdin is a way to explicitly ask swift for a REPL but it still places the REPL above other modes and it might conflict with changes I hope to make down the road. For example, if we move swift to a new, interactive help/search tool (something I would like to propose sooner rather than later), this would likely come around again. I think it would benefit incomparably more users for that to be a default command rather than sliding into the REPL. But I realize that is not yet concrete, so I would only argue at this point that the REPL should be a subcommand like everything else.

I totally take your point about breaking scripts but we have a sample size of one here and no details about what a script would actually be trying to accomplish by piping directly into the REPL. You mentioned that this "may" break scripts. Piping directly into the REPL in an automation context seems very esoteric to me–I would be super surprised if that was a dominant use case. The REPL is meant for a human being to enter explicitly. In that case, the swift repl subcommand is right there in the help, so it's pretty simple to adjust.

In this case, it seemed like you ran this manually but is that what happened? If so, were you just poking at the command to see if Swift was installed properly in the Docker container? Is there some other underlying concern?

@bitjammer
Copy link
Member

I actually tried changing the code to do that, and it did actually detect when its being piped into (ie, echo 'print("hi!")' | swift), however for whatever reason REPL immiedately starts & dies when triggering it..

Could this be because swift-frontend still catches all swift invocations in order to allow folks to fall back to the old, built-in driver with a flag? As I understand it, the frontend still execs to the new swift-driver as appropriate, something that bit me last year. To be clear, I really don't think this is what we should do, though.

@NSAntoine
Copy link
Contributor

I actually tried changing the code to do that, and it did actually detect when its being piped into (ie, echo 'print("hi!")' | swift), however for whatever reason REPL immiedately starts & dies when triggering it..

Could this be because swift-frontend still catches all swift invocations in order to allow folks to fall back to the old, built-in driver with a flag? As I understand it, the frontend still execs to the new swift-driver as appropriate, something that bit me last year. To be clear, I really don't think this is what we should do, though.

No idea, but it does sound like what you’re describing, as the process would end with a status 0 exit code immediately after starting REPL

@tomerd
Copy link
Member

tomerd commented May 13, 2022

@bitjammer the goal of separating the concerns is a good one IMO. that said, I suggest we provide a gradual transitional path. for example, if we can detect these invocations and emit a deprecation warning but still perform the script / maintain previous behavior for a full release then remove it, it may be more palatable for users that came to depend on this behavior

@bitjammer
Copy link
Member

So, I added the help intro when running the REPL back in September of last year and only recently made the final switch. Are you saying we should turn the REPL back on for another additional cycle?

@NSAntoine
Copy link
Contributor

So, I added the help intro when running the REPL back in September of last year and only recently made the final switch. Are you saying we should turn the REPL back on for another additional cycle?

I think the behaviour suggested by tomer would be a better option, keep it as it is right now however support piping with a warning if the script simply invokes swift rather than swift repl

@tomerd
Copy link
Member

tomerd commented May 14, 2022

right, I am suggesting we keep the new behavior, but when we detect a piped input we emit a deprecation warning and tee it off to repel to preserve previous behavior for one more cycle.

@NSAntoine
Copy link
Contributor

@tomerd Though I think piping should always be supported, but only to swift repl rather than just swift

@natecook1000
Copy link
Member

I'm surprised that piping into swift started the REPL; I would have expected it acted more like:

echo 'print("HELLO WORLD, THIS RUNS CODE.")' | cat > tmp.swift ; swift tmp.swift

i.e. compile and execute, which has slightly different semantics from REPL mode. @weissi would that solve your issue? It seems like the new welcome screen and changes to what's automatically printed would already cause problems for automated scripts.

@weissi
Copy link
Member Author

weissi commented May 17, 2022

I'm surprised that piping into swift started the REPL; I would have expected it acted more like:

echo 'print("HELLO WORLD, THIS RUNS CODE.")' | cat > tmp.swift ; swift tmp.swift

I think the canonical way which also doesn't need a file on disk is

echo 'print("hello")' | swift -

i.e. compile and execute, which has slightly different semantics from REPL mode. @weissi would that solve your issue? It seems like the new welcome screen and changes to what's automatically printed would already cause problems for automated scripts.

I don't personally rely on the old functionality, I was just pointing out that this is something that would break. I'm fine with changing it but I was slightly surprised by such a profound change w/o an evolution proposal.

@WowbaggersLiquidLunch
Copy link
Collaborator

Recently I've been confused by swift not launching REPL when using Xcode 14 beta command line tools. Because a similar block of text is printed every time when launching REPL in Swift 5.6, I basically ignored it and didn't notice the line instructing me to use swift repl, a mistake which I believe many other people will make and be confused by too. I found this discussion here only because I was going to file a bug report and wanted to search for any existing report first.

Although it probably hasn't been stated as a requirement for CLI changes to go through SE, there have been some precedents: SE-0085, SE-0179, and SE-0301. The previous discussion being in the "Evolution" category on the forums gives (me, at least) the impression that the discussed improvements will land via the Evolution process as well.

Maybe it's too much to ask for this change to go through SE, but I think this should have at least been an announcement on the forums or the blog.

@bitjammer
Copy link
Member

It was discussed on the forums here: https://forums.swift.org/t/command-line-ux-enhancements-for-swift/50670

@WowbaggersLiquidLunch
Copy link
Collaborator

It was discussed on the forums here: https://forums.swift.org/t/command-line-ux-enhancements-for-swift/50670

Yes, I know.

The previous discussion being in the "Evolution" category on the forums gives (me, at least) the impression that the discussed improvements will land via the Evolution process as well.

@bitjammer
Copy link
Member

Ah, sorry, I missed that part of your comment. I didn’t mean to imply anything by posting there necessarily, it seemed like the best place to announce the upcoming changes at the time. Will keep it in mind for the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. regression REPL
Projects
None yet
Development

No branches or pull requests

7 participants