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, part 1: Added interpreter mode to compiler interface, interpreter parsing functionality #64648

Closed
wants to merge 1 commit into from

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Sep 21, 2019

Summary:

  • Adds "interpreter mode" to compiler interface. This will be used later in several places, including diagnostics.
  • Adds "interpreter tag" to ast::Locals to track info like whether they came from a previous eval session or have been moved.
  • Added interface for injecting a pre-parsed "user body" into the parsing pipeline. cf. TyCtxt::get_interp_user_fn, expr.rs
  • Moved Steal data structure to rustc_data_structures crate since a) it was already used outside of rustc::ty crate, b) it's now used even a little more outside there.
  • Made a few more things pub (as little as possible), so the interpreter can use them.

If you want the big picture of where this is going in terms of compiler changes (probably 2/3 more PRs needed), take a look at my personal branch. I will also be publishing the REPL repo itself soon.

Also, sorry for the lack of commits; I basically just carved this out of an even bigger squashed commit after much, much hacking! (It might be a tad heavy on cosmetic stuff too, for the same reason. If it's okay, then great, otherwise I can try to revert certain areas that people really don't want.)

Maybe @Centril / @Zoxc for review? Not wholly sure.

CC @nikomatsakis @mw @eddyb

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2019
@alexreg
Copy link
Contributor Author

alexreg commented Sep 21, 2019

r? @Centril (provisional)

@rust-highfive rust-highfive assigned Centril and unassigned zackmdavis Sep 21, 2019
@alexreg alexreg changed the title REPL: Added interpreter mode to compiler interface, interpreter parsing functionality REPL, part 1: Added interpreter mode to compiler interface, interpreter parsing functionality Sep 21, 2019
@rust-highfive

This comment has been minimized.

src/librustc/ty/context.rs Outdated Show resolved Hide resolved
src/librustc/ty/context.rs Outdated Show resolved Hide resolved
src/librustc/ty/context.rs Outdated Show resolved Hide resolved
src/librustc_interface/interface.rs Outdated Show resolved Hide resolved
src/librustc_interface/queries.rs Outdated Show resolved Hide resolved
src/libsyntax/feature_gate/builtin_attrs.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/expr.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Sep 21, 2019

I left some thoughts above but most of this is outside my wheelhouse.

r? @Zoxc

@rust-highfive

This comment has been minimized.

@bjorn3

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 23, 2019

☔ The latest upstream changes (presumably #64695) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum
Copy link
Member

I would personally prefer we split the cosmetic changes out of this PR since it's orthogonal and can land separately (and is likely to generate more review traffic... after all, we're painting a shed :)

src/librustc/ty/context.rs Outdated Show resolved Hide resolved
src/librustc/ty/context.rs Outdated Show resolved Hide resolved
src/librustc_data_structures/steal.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Show resolved Hide resolved
src/libsyntax/parse/parser/expr.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/expr.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/expr.rs Outdated Show resolved Hide resolved
src/test/ui/reify-intrinsic.stderr Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

I think we might want T-compiler or so to provisionally agree to this -- it feels like the kind of thing we might want an eRFC (at least) for, but I also see why you might want to avoid that process.

Modulo possibly wanting an RFC, and splitting out the cosmetic change commit, I think that I at least feel that this PR is good enough (with my review comments addressed).

@Centril
Copy link
Contributor

Centril commented Sep 26, 2019

An RFC feels rather overkill. We didn't have ones for Clippy or Miri when they wanted to hook into the compiler and I don't see why we'd need one now.

@alexreg
Copy link
Contributor Author

alexreg commented Sep 26, 2019

@Mark-Simulacrum Thanks for the review!

I very much agree with @Centril about not needing an RFC for this (I'm naturally disinclined to them for minor changes and experimentation, and I think @nikomatsakis feels the same. Also with @Centril a.k.a. the unofficial King of RFCs is typically more inclined to them, so if even he doesn't want one...).

Let me address your comments as much as possible over the next day (I left some replies too), which mainly make sense to me, and then you can have another look if that's alright. As for cosmetic stuff, some of them are very much "intertwined" with the functionality code, and I feel it would be a pain to factor them out... also, there's a policy specifically against accepting separate cosmetic PRs these days. So I'd implore you to let these stay in, if possible, although if you/other team members want me to trim them down, let me know which ones, and I can probably do that without much of a problem. :-)

@Mark-Simulacrum
Copy link
Member

Left some replies to your comments. If you ping me on Zulip I'll almost certainly respond quickly most of the time, Discord is a bit more flaky with notifications; but either works if you want to talk about anything I've said synchronously.

Yeah, the eRFC/RFC comment was mostly for the larger feature, I agree we don't need it for the initial experimentation, if it has little impact on compiler and such.

Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
…crum

REPL, part 1: Added interpreter mode to compiler interface, interpreter parsing functionality

Summary:
* Adds "interpreter mode" to compiler interface. This will be used later in several places, including diagnostics.
* Adds "interpreter tag" to `ast::Local`s to track info like whether they came from a previous eval session or have been moved.
* Added interface for injecting a pre-parsed "user body" into the parsing pipeline. cf. `TyCtxt::get_interp_user_fn`, `expr.rs`
* Moved `Steal` data structure to `rustc_data_structures` crate since a) it was already used outside of `rustc::ty` crate, b) it's now used even a little more outside there.
* Made a few more things `pub` (as little as possible), so the interpreter can use them.

If you want the big picture of where this is going in terms of compiler changes (probably 2/3 more PRs needed), take a look at my [personal branch](https://github.com/alexreg/rust/tree/rush). I will also be publishing the REPL repo itself soon.

Also, sorry for the lack of commits; I basically just carved this out of an even bigger squashed commit after much, much hacking! (It might be a tad heavy on cosmetic stuff too, for the same reason. If it's okay, then great, otherwise I can try to revert certain areas that people really don't want.)

Maybe @Centril / @Zoxc for review? Not wholly sure.

CC @nikomatsakis @mw @eddyb
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
…crum

REPL, part 1: Added interpreter mode to compiler interface, interpreter parsing functionality

Summary:
* Adds "interpreter mode" to compiler interface. This will be used later in several places, including diagnostics.
* Adds "interpreter tag" to `ast::Local`s to track info like whether they came from a previous eval session or have been moved.
* Added interface for injecting a pre-parsed "user body" into the parsing pipeline. cf. `TyCtxt::get_interp_user_fn`, `expr.rs`
* Moved `Steal` data structure to `rustc_data_structures` crate since a) it was already used outside of `rustc::ty` crate, b) it's now used even a little more outside there.
* Made a few more things `pub` (as little as possible), so the interpreter can use them.

If you want the big picture of where this is going in terms of compiler changes (probably 2/3 more PRs needed), take a look at my [personal branch](https://github.com/alexreg/rust/tree/rush). I will also be publishing the REPL repo itself soon.

Also, sorry for the lack of commits; I basically just carved this out of an even bigger squashed commit after much, much hacking! (It might be a tad heavy on cosmetic stuff too, for the same reason. If it's okay, then great, otherwise I can try to revert certain areas that people really don't want.)

Maybe @Centril / @Zoxc for review? Not wholly sure.

CC @nikomatsakis @mw @eddyb
@Centril
Copy link
Contributor

Centril commented Oct 1, 2019

Failed in #64954 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 1, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Oct 1, 2019

Before merging this we'll do a design meeting of the compiler team to figure out how and whether the compiler should be modified in order to help creating a REPL. Relevant discussion happened on zulip.

@oli-obk oli-obk added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Oct 1, 2019
@Mark-Simulacrum
Copy link
Member

r? @oli-obk on this PR as the compiler team driver for now then

@alexreg
Copy link
Contributor Author

alexreg commented Oct 1, 2019

Yeah, just noticed this now. Thanks for your reviews anyway @Mark-Simulacrum! Hopefully we can get it merged soon, regardless (at least in a form close to the present one).

@bors
Copy link
Contributor

bors commented Oct 2, 2019

☔ The latest upstream changes (presumably #64981) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

Hey all -- sorry for being silent. @alexreg pointed me at this a while back but I've been behind. In general, I agree that a design meeting would be great and appropriate. Earlier, @alexreg wrote:

If you want the big picture of where this is going in terms of compiler changes (probably 2/3 more PRs needed), take a look at my personal branch. I will also be publishing the REPL repo itself soon.

I think that having an idea of the bigger target is exactly the kind of thing we should talk about, but unfortunately I don't really have the time to read a branch and try to "reverse engineer" what the larger design is from that. What I would like to see is probably a more elaborated version of the "summary" from this PR, that tries to show how the pieces will fit together in the end.

I'm not sure yet overall whether I want to land support for a REPL. I think a REPL would be a great thing. But we have a lot of stuff going on and a lot of priorities. So I guess it's partly a question of how "invasive" the support is, and what kind of ongoing support it seems likely to require. My impression from this PR is that it should be relatively minimal? This seems like the kind of information that we would be looking for in evaluating the design doc.

(I'd also like the design doc to be something that could move to rustc-guide to help people in understanding the changes that are being done. I personally find it really hard to review commits when I don't have a reasonable up-front idea of what the PR is trying to do.)

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 9, 2019

@alexreg to be clear, the design meeting process is pretty lightweight! Just file an issue on the compiler-team repo. We can also schedule it at a time that's not the normal slot, if that slot doesn't work well for you.

@alexreg
Copy link
Contributor Author

alexreg commented Oct 10, 2019

@nikomatsakis

I think that having an idea of the bigger target is exactly the kind of thing we should talk about, but unfortunately I don't really have the time to read a branch and try to "reverse engineer" what the larger design is from that. What I would like to see is probably a more elaborated version of the "summary" from this PR, that tries to show how the pieces will fit together in the end.

That's fair. And I didn't expect you to do as much. I just wrote this to say "hey, the branch is there in case you're curious". What I can do is write up an expanded summary doc for this PR and the upcoming ones, and the overall vision. On HackMD or something.

I'm not sure yet overall whether I want to land support for a REPL. I think a REPL would be a great thing. But we have a lot of stuff going on and a lot of priorities. So I guess it's partly a question of how "invasive" the support is, and what kind of ongoing support it seems likely to require. My impression from this PR is that it should be relatively minimal? This seems like the kind of information that we would be looking for in evaluating the design doc.

I can understand why, but I think if the intrusion into the rustc compiler is minimal (as I've planned it to be), then why not? I'm happy to take on "maintenance duties" as far as the few REPL-only bits of rustc are concerned. I'm convinced this PR is pretty unobtrusive, so I'm glad you agree there so far.

(I'd also like the design doc to be something that could move to rustc-guide to help people in understanding the changes that are being done. I personally find it really hard to review commits when I don't have a reasonable up-front idea of what the PR is trying to do.)

Yeah, I'll keep that in mind when writing it. If it's on HackMD, then others like you can add notes and adjust things here and there if you fancy... no obligation though.

@alexreg to be clear, the design meeting process is pretty lightweight! Just file an issue on the compiler-team repo. We can also schedule it at a time that's not the normal slot, if that slot doesn't work well for you.

Ah, didn't know, but sounds good. If we could somehow schedule a short one next week (giving me time to prepare this doc, and people to see it, but not so longer as to let this PR linger more or even fester), then perfect.

@mark-i-m
Copy link
Member

Ah, didn't know, but sounds good. If we could somehow schedule a short one next week (giving me time to prepare this doc, and people to see it, but not so longer as to let this PR linger more or even fester), then perfect.

@alexreg I believe the design meetings are planned out a month in advance (e.g. the next three weekly meeting topics have already been chosen). I might be wrong though... I've mostly been lurking...

Btw, this is really cool, and I would love to see a repl happen...

@nikomatsakis
Copy link
Contributor

We pick the "whole compiler" design meetings every 4 weeks, yes, but we're part-way through the cycle. @alexreg if you file the issue now we'd be picking the next set on October 25th. You can read more about the process on the compiler-team web page.

/// In interpreter mode, parses the placeholder for the user fn body as the user-provided
/// block (not from the source).
/// Outside of interpreter mode, simply returns `Ok(None)`.
fn parse_interp_user_fn_block(&mut self) -> PResult<'a, Option<P<Block>>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminder: check for interp mode here, maybe?

@Dylan-DPC-zz Dylan-DPC-zz removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 18, 2019
@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Mar 6, 2020

I'm closing this due to inactivity and the large number of conflicts. Feel free to reopen it (or preferably create a new PR ) when you have progress :) Thanks Nevermind, didn't realise it was blocked

@Dylan-DPC-zz Dylan-DPC-zz reopened this Mar 6, 2020
@bjorn3
Copy link
Member

bjorn3 commented Mar 6, 2020

The design meetong referenced in #64648 (comment) has already happened a while ago.

@bors
Copy link
Contributor

bors commented May 1, 2020

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout rush-parsing (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self rush-parsing --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
CONFLICT (modify/delete): src/libsyntax_pos/symbol.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax_pos/symbol.rs left in tree.
CONFLICT (modify/delete): src/libsyntax_pos/span_encoding.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax_pos/span_encoding.rs left in tree.
CONFLICT (modify/delete): src/libsyntax_ext/deriving/debug.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax_ext/deriving/debug.rs left in tree.
CONFLICT (modify/delete): src/libsyntax/parse/parser/stmt.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax/parse/parser/stmt.rs left in tree.
CONFLICT (modify/delete): src/libsyntax/parse/parser/expr.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax/parse/parser/expr.rs left in tree.
CONFLICT (modify/delete): src/libsyntax/parse/parser.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax/parse/parser.rs left in tree.
CONFLICT (modify/delete): src/libsyntax/parse/mod.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax/parse/mod.rs left in tree.
CONFLICT (modify/delete): src/libsyntax/parse/diagnostics.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax/parse/diagnostics.rs left in tree.
CONFLICT (modify/delete): src/libsyntax/mut_visit.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax/mut_visit.rs left in tree.
CONFLICT (modify/delete): src/libsyntax/feature_gate/builtin_attrs.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax/feature_gate/builtin_attrs.rs left in tree.
CONFLICT (modify/delete): src/libsyntax/ext/build.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax/ext/build.rs left in tree.
CONFLICT (modify/delete): src/libsyntax/ast.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/libsyntax/ast.rs left in tree.
Auto-merging src/librustc_mir/transform/mod.rs
CONFLICT (content): Merge conflict in src/librustc_mir/transform/mod.rs
Auto-merging src/librustc_interface/queries.rs
CONFLICT (content): Merge conflict in src/librustc_interface/queries.rs
Auto-merging src/librustc_interface/passes.rs
CONFLICT (content): Merge conflict in src/librustc_interface/passes.rs
Auto-merging src/librustc_interface/lib.rs
CONFLICT (content): Merge conflict in src/librustc_interface/lib.rs
Auto-merging src/librustc_interface/interface.rs
CONFLICT (content): Merge conflict in src/librustc_interface/interface.rs
CONFLICT (rename/delete): src/librustc/ty/steal.rs deleted in HEAD and renamed to src/librustc_data_structures/steal.rs in heads/homu-tmp. Version heads/homu-tmp of src/librustc_data_structures/steal.rs left in tree.
Auto-merging src/librustc_data_structures/lib.rs
CONFLICT (content): Merge conflict in src/librustc_data_structures/lib.rs
CONFLICT (modify/delete): src/librustc/ty/query/mod.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/ty/query/mod.rs left in tree.
CONFLICT (modify/delete): src/librustc/ty/mod.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/ty/mod.rs left in tree.
CONFLICT (modify/delete): src/librustc/ty/context.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/ty/context.rs left in tree.
CONFLICT (modify/delete): src/librustc/session/config.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/session/config.rs left in tree.
CONFLICT (modify/delete): src/librustc/query/mod.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/query/mod.rs left in tree.
CONFLICT (modify/delete): src/librustc/ich/impls_ty.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/ich/impls_ty.rs left in tree.
CONFLICT (modify/delete): src/librustc/ich/impls_hir.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/ich/impls_hir.rs left in tree.
CONFLICT (modify/delete): src/librustc/hir/mod.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/hir/mod.rs left in tree.
CONFLICT (modify/delete): src/librustc/hir/lowering.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/hir/lowering.rs left in tree.
CONFLICT (modify/delete): src/librustc/arena.rs deleted in HEAD and modified in heads/homu-tmp. Version heads/homu-tmp of src/librustc/arena.rs left in tree.
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at least 3410 and retry the command.
Automatic merge failed; fix conflicts and then commit the result.

@nikomatsakis
Copy link
Contributor

I'm going to close this for inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.