Modifications to rustc for the Rust REPL

[alexreg]

As several of you know, I managed to successfully create a proof-of-concept Rust REPL about a couple of months ago. This necessitated (at least, given a clean and sane approach) a few — essentially minor — changes to rustc.

As discussed with @nikomatsakis and @Mark-Simulacrum, the we thought the best way to proceed with this would be to create a design document that summarises my approach to creating the REPL, along with some technical details. The idea is to get some feedback either before or during the FCP process, from which I can proceed by submitting PRs.

HackMD Document

See the first REPL-related PR (currently pending this issue and FCP) for more examples of the sorts of changes I am making (although that will need to be updated somewhat).

[Mark-Simulacrum]

I have a few questions I'd like to see answered, reading over the HackMD. It looks like a few of the main bullets under "modifications to rustc" look rather large -- e.g., incremental compilation's VFS -- and seem sort of irrelevant (I'd hope) to making a REPL work. Could we start by getting a set of changes that allow minimal functionality?

It seems to me that many of the changes you're making aren't strictly necessary, though may be beneficial performance/usability wise. For example, I think the diagnostic and type printing changes I'd personally be against, at least initially, since they are pretty "invasive" and open ended as well as being not too helpful in general: our diagnostics are hopefully already pretty good.

My main concern is that it looks like the proposal as written is sort of quite extensive -- touching on many areas of the compiler -- and while that may all be necessary, it does seem like it'd be good to try to scale back and aim for an MVP rather than shooting for a 99% solution immediately, so as to get the project off the ground.

It also seems like we could hopefully avoid at least some of the changes to AST types, etc. -- you comment on what the change enables, but not much as to what the alternative (i.e., without the change) would be. If the alternative is worse diagnostics I would like to see it dropped, at least for now, since diagnostics can always be improved later (and perhaps in a way that would be independent of whether we're in REPL mode or not).

[alexreg]

@Mark-Simulacrum

I have a few questions I'd like to see answered, reading over the HackMD. It looks like a few of the main bullets under "modifications to rustc" look rather large -- e.g., incremental compilation's VFS -- and seem sort of irrelevant (I'd hope) to making a REPL work. Could we start by getting a set of changes that allow minimal functionality?

I don't think they're that big as features to be honest. The VFS one is easily the largest I'd say, and even then it's pretty self-contained. It's not absolutely essential I suppose, but do to the way I've structured my REPL, it would be very beneficial. Also, this was suggested by Zoxc when I chatted with him online some time ago, and this would appear to be very much in his domain of expertise within the codebase.

It seems to me that many of the changes you're making aren't strictly necessary, though may be beneficial performance/usability wise. For example, I think the diagnostic and type printing changes I'd personally be against, at least initially, since they are pretty "invasive" and open ended as well as being not too helpful in general: our diagnostics are hopefully already pretty good.

Honestly, no. The diagnostics changes are very much necessary, otherwise you can end up inputting separate lines to the REPL (separate evaluation sessions), and if a variable is moved in a previous line (or was was declared but never defined), and it appears like it simply never existed rather than got moved.

My main concern is that it looks like the proposal as written is sort of quite extensive -- touching on many areas of the compiler -- and while that may all be necessary, it does seem like it'd be good to try to scale back and aim for an MVP rather than shooting for a 99% solution immediately, so as to get the project off the ground.

I don't see why. I've put in all the hard graft, and got a working prototype. I'd like to see that work come to fruition and not wasted, given the intrusion into the codebase is pretty minimal in most cases, and quite localised.

It also seems like we could hopefully avoid at least some of the changes to AST types, etc. -- you comment on what the change enables, but not much as to what the alternative (i.e., without the change) would be. If the alternative is worse diagnostics I would like to see it dropped, at least for now, since diagnostics can always be improved later (and perhaps in a way that would be independent of whether we're in REPL mode or not).

The alternative would be something hacky (and requiring more code) like embedding this information in custom attributes on locals. Like I said above, these diagnostics (just two: uninitiasliazed and moved locals) are fundamental to the operation of the REPL, really.

[Mark-Simulacrum]

You say that the diagnostic changes, etc. are fundamental to the operation of the REPL, but that is confusing to me: how is a better diagnostic fundamental to this operation? I presume that the REPL would still work -- just worse (in the sense that the user experience would be less good). When you say "and if a variable is moved in a previous line (or was was declared but never defined), and it appears like it simply never existed rather than got moved." that seems like a "fine" UX for the initial prototype, though it also seems odd to me (not how I'd expect the feature to work).

I am mainly hesitant here because it sounds like you've done a lot of work (which is great!) but sort of in somewhat of an echo chamber, and if we're going to semi-endorse this specific REPL implementation and make it's design somewhat 'official' we'd want to make sure that's what we want to do. I'm super excited to have this implementation, but I want to make sure we're careful about how we do it, so if that means getting a minimal thing working and then going from there, that seems much better.

I don't see why. I've put in all the hard graft, and got a working prototype. I'd like to see that work come to fruition and not wasted, given the intrusion into the codebase is pretty minimal in most cases, and quite localised.

I think it would be great -- I probably don't have the time myself, unfortunately -- but to take the repository for the REPL and try and edit it a bit in such a way that it requires fewer or no changes on the part of rustc. I expect that's possible (at the loss of performance, presumably, along with nice diagnostics), though it is somewhat obviously more work than just utilizing your existing patch set. It basically sums up to: if you can get off the ground sooner by dropping the things the compiler team needs to approve, that seems like it'll happen much sooner than if we try to push through all of this (I get it's not much, but to my knowledge it's the first time we've done something like this, so we're operating in sort of an open zone without too much guidance from preexisting actions).

[alexreg]

@Mark-Simulacrum

You say that the diagnostic changes, etc. are fundamental to the operation of the REPL, but that is confusing to me: how is a better diagnostic fundamental to this operation? I presume that the REPL would still work -- just worse (in the sense that the user experience would be less good). When you say "and if a variable is moved in a previous line (or was was declared but never defined), and it appears like it simply never existed rather than got moved." that seems like a "fine" UX for the initial prototype, though it also seems odd to me (not how I'd expect the feature to work).

Well, upon rethinking this, I suppose you have a point: it depends what exactly you consider "fundamental". You wouldn't get ICEs or unsound behaviour, I suppose, but you'd get very surprising and unhelpful behaviour.

I think it would be great -- I probably don't have the time myself, unfortunately -- but to take the repository for the REPL and try and edit it a bit in such a way that it requires fewer or no changes on the part of rustc. I expect that's possible (at the loss of performance, presumably, along with nice diagnostics), though it is somewhat obviously more work than just utilizing your existing patch set. It basically sums up to: if you can get off the ground sooner by dropping the things the compiler team needs to approve, that seems like it'll happen much sooner than if we try to push through all of this (I get it's not much, but to my knowledge it's the first time we've done something like this, so we're operating in sort of an open zone without too much guidance from preexisting actions).

Which is fair enough, but I do want to stress these changes to diagnostics and whatnot are really not very big. They actually don't represent many extra lines of code at all. That said, as a compromise, perhaps we can mark them as "optional" or "deferred" in this design doc, and aim to create those PRs and get them merged after the essential modifications to rustc?

I am mainly hesitant here because it sounds like you've done a lot of work (which is great!) but sort of in somewhat of an echo chamber, and if we're going to semi-endorse this specific REPL implementation and make it's design somewhat 'official' we'd want to make sure that's what we want to do. I'm super excited to have this implementation, but I want to make sure we're careful about how we do it, so if that means getting a minimal thing working and then going from there, that seems much better.

I'm certainly grateful for the recognition of what I've put into this (indeed, a lot of experimentation and learning!). Perhaps my above suggested compromise could work? I could then prepare a PR for those things like diagnostic improvements and you can see how moderate they really are (I hope!). Also, while you're right that I've done the vast majority of this work by myself, as is natural for any research-oriented/experimentation work in this domain, especially when others' time is limited, I have consulted the most knowledgeable compiler team members in their respective areas while working on this (including RalfJ and oli-obk for MIR stuff, Zoxc for passes and incr. comp., nikomatsakis, and Centril for a few design points). So yeah, 'echo chamber' probably isn't totally fair, but nor exactly has it been a team effort. Semi-official support is indeed what I think would be best at this point. :-)

[Mark-Simulacrum]

Well, upon rethinking this, I suppose you have a point: it depends what exactly you consider "fundamental". You wouldn't get ICEs or unsound behaviour, I suppose, but you'd get very surprising and unhelpful behaviour.

Yep, this was what I was thinking basically -- I agree that the behavior might be very non-ideal, but if it works, we can iterate from there rather than saying "all of this is blocking" or so :)

That said, as a compromise, perhaps we can mark them as "optional" or "deferred" in this design doc, and aim to create those PRs and get them merged after the essential modifications to rustc?

I think this is perfect! My goal here is to eke out the minimum necessary to get us to a point where people can legitimately be using the REPL, even with not great diagnostics and such, and then we can make enhancements from there. I think minimizing scope of work is a great way to do this, and it seems like you agree.

I don't think that the diagnostic changes are big, to be clear, it's just that I think that it's a bit unusual for us to be accepting changes to make other projects viable into the tree proper, which is why I think it's best for us to be cautious here and not just land them immediately, even if the changes are small. There's a cost to untested code in the tree, even if it's small. Partially I also want to believe that we can come up with a design that doesn't need them, to be clear, which is why I'm pushing back a little against adding them -- once they're in, there's much less desire to think about things I imagine :)

So yeah, 'echo chamber' probably isn't totally fair, but nor exactly has it been a team effort.

I want to be clear that I meant that in a positive way, i.e., I'm sure there's been some help from many parties and such, but you've probably not had feedback of the deep design kind (for good reason!).

[pnkfelix]

cc rust-lang/rust#1120 rust-lang/rust#9898 and rust-lang/rfcs#655

[alexreg]

@pnkfelix Thanks for those links. Good to have for reference, although worth noting that the approach I decided to take here, in large part for ease of implementation, was to leverage miri rather than the LLVM and associated JIT machinery. (This was mainly on the advice of oli-obk and Centril, some time ago.)

[alexreg]

@Mark-Simulacrum

I think this is perfect! My goal here is to eke out the minimum necessary to get us to a point where people can legitimately be using the REPL, even with not great diagnostics and such, and then we can make enhancements from there. I think minimizing scope of work is a great way to do this, and it seems like you agree.

Okay great. We're on the same page here.

I don't think that the diagnostic changes are big, to be clear, it's just that I think that it's a bit unusual for us to be accepting changes to make other projects viable into the tree proper, which is why I think it's best for us to be cautious here and not just land them immediately, even if the changes are small. There's a cost to untested code in the tree, even if it's small. Partially I also want to believe that we can come up with a design that doesn't need them, to be clear, which is why I'm pushing back a little against adding them -- once they're in, there's much less desire to think about things I imagine :)

Indeed, they're not too big, though I appreciate we're in a novel situation with this "semi-official" support (modifying the compiler in relatively minimal ways to accommodate an outside project). I don't believe it's totally novel, given projects like miri (the binary) and priroda, but yeah, it's certainly not common.

I want to be clear that I meant that in a positive way, i.e., I'm sure there's been some help from many parties and such, but you've probably not had feedback of the deep design kind (for good reason!).

No problem, I wasn't offended... and yes, for obvious reasons, I've not had a lot of the intricacies of the good reviewed yet. But this issue is mainly to discuss the higher-level design points and then approve them, hopefully. :-)

[alexreg]

@nikomatsakis Would be great to get your feedback at this point. Namely, any concerns you have, and whether you agree on what @Mark-Simulacrum and I have been discussing above (besides the original post & design doc). After that, would be great to move to FCP so this doesn't get bogged down. Equally, I appreciate you and others will want to know we're making reasonable decisions here, even if none of these modifications is exactly ground-sweeping!

[Mark-Simulacrum]

I think updating the hack md ( https://hackmd.io/GJokfI0wQ0i4SIgRbFTmfw) with the changes we've discussed by splitting the changes requested to rustc into two categories (one essential, very small, if anything and one non-essential) would be a prereq for further progress. For now we would just FCP the essential changes I imagine.

[alexreg]

@Mark-Simulacrum Yep I'll do that tomorrow, for sure. Niko can regardless review whenever he has time, I hope. It would be nice to see what he thinks of the non-essential changes too, just for when they come up at a later point.

[alexreg]

@Mark-Simulacrum Updated!

[alexreg]

Updated the design doc again.

@nikomatsakis Would you mind giving this a read-over again, so we can ideally move forward and FCP this ASAP? :-) @Mark-Simulacrum and I have discussed a few things on Zulip, and I have even shared the REPL repository source code with him. I gathered that he was pretty satisfied with the state of the design and sees where I'm coming from with all my choices (at least in the updated doc). Hopefully you do too!

[Mark-Simulacrum]

To be clear I'm not sure that I'd say I'm satisfied, I just wasn't able to come up with anything major that I could see easy ways to change in the ~3 hours I spent on this over the weekend. So in some sense I'm not too happy, but I also don't think blocking on "this seems not great" makes sense; so I don't mind moving forward provisionally myself. I do think broader compiler team discussion -- perhaps at a design meeting -- makes a lot of sense.

[alexreg]

Okay, sorry if I misunderstood. You're "happy enough not to block it" then, I guess.

[nikomatsakis]

Scheduled the meeting for Friday, Nov 29.

[bjorn3]

How hard would it be to extend this to using cg_clif as backend? It already has basic JIT support (only dylib deps) and Cranelift tracks the location of all live values.

[alexreg]

@bjorn3 Probably fairly non-trivial, given this is quite dependent on miri. That said, one this gets merged, I'd be glad to work with you on adding Cranelift as an alternative backend, if you're interested? This would lay at least some of the groundwork for that. It would be super to have two backends, Miri more for correctness, advanced debugging, and possibly other bonuses, and Cranelift for speed.

Maybe drop by in the meeting tomorrow, and we'll discuss that towards the end if we have time (which is very possible)?

[bjorn3]

When (UTC time) and where will be the meeting?

[bjorn3]

Probably fairly non-trivial, given this is quite dependent on miri.

I understand.

I'd be glad to work with you on adding Cranelift as an alternative backend, if you're interested?

Yes, but I don't know how many time I can spend on it though.

[bjorn3]

When (UTC time) and where will be the meeting?

Looked at the agenda from #213 (comment). If it shows the time in my native time-zone, I think I will be able to attend it for some time.

[bjorn3]

I had deactivated my zulip account in the past. Can somebody reactivate it? According to the zulip help only an administrator can.

https://zulipchat.com/help/deactivate-your-account
https://zulipchat.com/help/deactivate-or-reactivate-a-user#reactivate-a-user

[alexreg]

Yes, but I don't know how many time I can spend on it though.

No problem. If we can do some planning together, and maybe you review some of the Cranelift-specific parts, that could probably work. (I will have to learn more about Cranelift, but that's no problem. This is a lot better than an LLVM REPL I reckon!)

[bjorn3]

If we can do some planning together, and maybe you review some of the Cranelift-specific parts, that could probably work.

👍 I can review the cg_clif specific parts. I doubt there will need to be big changes to Cranelift itself, as it is already made to be integrated in a JIT (spidermonkey)

I will have to learn more about Cranelift, but that's no problem.

You can ask me if you have any questions.

[bjorn3]

My zulip account has just been re-enabled. Thanks @davidtwco!

[alexreg]

Sounds good, @bjorn3. Hopefully we can get a prototype of the miri-based version working first, and then I can bring my attention to the Cranelift backend, which I'd certainly like to implement with your help.

[mark-i-m]

Is there a repo or tracking issue where one can follow this work?

[alexreg]

@mark-i-m The meeting minutes summarise the current state of things insofar as design, though there have been some further discussions on Zulip (topics beginning with REPL: under the #t-compiler stream). Over the coming weeks I'll be reworking my prototype to conform fully with the design doc and what was discussed (also further conclusions that were reached or are still being discussed on Zulp). Maybe I can create a topic where I can post announcements about this, e.g., new PRs and whatnot.

[mark-i-m]

Maybe I can create a topic where I can post announcements about this, e.g., new PRs and whatnot.

@alexreg Thanks! That would be great :)