Skip to content
This repository has been archived by the owner on Jun 3, 2021. It is now read-only.

Implement proper REPL #515

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Implement proper REPL #515

wants to merge 10 commits into from

Conversation

Stupremee
Copy link
Contributor

It's the last pr for a repl, I swear 😅

Resolves #372

jyn514
jyn514 reviewed Sep 8, 2020
View reviewed changes
Copy link
Owner

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Hmm, the other workspace members don't use a nested src directory. I'm not super tied to that, but is there a reason you added it here? The only thing at the top-level looks like Cargo.toml

saltwater-repl/src/commands.rs Outdated Show resolved Hide resolved
saltwater-repl/src/commands.rs Outdated Show resolved Hide resolved
saltwater-repl/src/helper.rs Outdated Show resolved Hide resolved
saltwater-repl/src/main.rs Outdated Show resolved Hide resolved
@Stupremee
Copy link
Contributor Author

Hmm, the other workspace members don't use a nested src directory. I'm not super tied to that, but is there a reason you added it here? The only thing at the top-level looks like Cargo.toml

Ah I see, that's just how cargo new generated it. I will change it 👍

@jyn514

This comment has been minimized.

Sign in to view
@Stupremee
Copy link
Contributor Author

Yea the first error is fixed by my latest commit, but the other error is weird

@Stupremee Stupremee marked this pull request as ready for review September 13, 2020 15:17
@jyn514
Copy link
Owner

jyn514 commented Sep 13, 2020 

error[E0063]: missing field `jit` in initializer of `saltwater_parser::Opt`
   --> src/main.rs:361:14
    |
361 |         opt: Opt {
    |              ^^^ missing `jit`

You're passing --jit to the codegen crate, but not to the saltwater binary. I think this only popped up now because previously clippy was ignoring the jit feature completely for codegen. I'll fix this, one sec.

jyn514 added a commit that referenced this pull request Sep 13, 2020
@jyn514
Run clippy on non-default features, too
98f678e 
This fixes the CI failure in
#515 (comment) and
is generally good practice.
@jyn514 jyn514 mentioned this pull request Sep 13, 2020
Merged
jyn514 added a commit that referenced this pull request Sep 13, 2020
@jyn514
Run clippy on non-default features, too
3ad8462 
This fixes the CI failure in
#515 (comment) and
is generally good practice.
jyn514
jyn514 reviewed Sep 13, 2020
View reviewed changes
saltwater-codegen/lib.rs
@@ -345,6 +345,44 @@ impl<B: Backend> Compiler<B> {

pub type Product = <cranelift_object::ObjectBackend as Backend>::Product;

/// Compiles a single declaration.
pub fn compile_decl<B: Backend>(
Copy link
Owner

Choose a reason for hiding this comment

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

This looks copy/pasted from https://github.com/jyn514/saltwater/pull/515/files#diff-376fa271c6951618907ff944ff30e93aL368. Rather than duplicating the code, can you change that to call this function instead? That way they're consistent.

saltwater-parser/analyze/mod.rs Show resolved Hide resolved
saltwater-repl/Cargo.toml Outdated Show resolved Hide resolved
saltwater-repl/commands.rs Outdated Show resolved Hide resolved
saltwater-repl/commands.rs Outdated Show resolved Hide resolved
saltwater-repl/repl.rs Outdated Show resolved Hide resolved
saltwater-repl/repl.rs Outdated Show resolved Hide resolved
saltwater-repl/commands.rs Outdated Show resolved Hide resolved
saltwater-repl/main.rs
match repl.run() {
Ok(_) => {}
Err(err) => {
println!("error: {}", err);
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, these errors are only sometimes showing up.

>> "a";
error: expression returns unsupported type: Array(Char(true), Fixed(2))
>> @
# I pressed ctrl+c
>> x = 1

>>

Copy link
Contributor Author

@Stupremee Stupremee Sep 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

These lines only print the error that is returned by rustyline if something terminal related fails.
The input isn't valid because the Validator checks if the input is a valid expression (which fails because x is not declared, I need to fix that).

Imo it's better to remove the "only expressions are valid input" check because it can be really confusing

Copy link
Owner

Choose a reason for hiding this comment

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

Right, but this isn't reporting the error at all, which is a bug. Do you know how to fix that? I'm not sure entirely where the syntax errors are bubbling up - maybe in execute_code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The errors are reported in process_line

saltwater-repl/repl.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Owner

jyn514 commented Sep 13, 2020

Opened bytecodealliance/wasmtime#2199

jyn514
jyn514 reviewed Sep 13, 2020
View reviewed changes
saltwater-repl/commands.rs Show resolved Hide resolved
saltwater-repl/helper.rs Outdated Show resolved Hide resolved
saltwater-repl/repl.rs Outdated Show resolved Hide resolved
saltwater-repl/repl.rs Outdated Show resolved Hide resolved
saltwater-repl/main.rs
match repl.run() {
Ok(_) => {}
Err(err) => {
println!("error: {}", err);
Copy link
Owner

Choose a reason for hiding this comment

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

Right, but this isn't reporting the error at all, which is a bug. Do you know how to fix that? I'm not sure entirely where the syntax errors are bubbling up - maybe in execute_code?

@Stupremee
Copy link
Contributor Author

Should I wait until target-lexicon is updated?

@jyn514
Copy link
Owner

jyn514 commented Sep 13, 2020

For now let's just add #[cfg(not(target_pointer_width = "64"))] compile_error!("only x86_64 is supported");, and then we can make it a run-time error instead once cranelift is updated.

jyn514
jyn514 reviewed Sep 13, 2020
View reviewed changes
saltwater-repl/repl.rs
Ok(_) => {}
Err(err) => {
// TODO: Proper error reporting
println!("error: {}", err.data);
Copy link
Owner

Choose a reason for hiding this comment

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

This bit doesn't seem to be working (as mentioned above):

>> @
;
# no output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it's not a valid expression and thus get declined by rustyline.

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

@jyn514 jyn514 jyn514 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use rustyline for REPL
2 participants
@Stupremee @jyn514