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

Only raise RuntimeErrors from the Interpreter #98

Closed
faultyserver opened this issue Dec 26, 2017 · 0 comments
Closed

Only raise RuntimeErrors from the Interpreter #98

faultyserver opened this issue Dec 26, 2017 · 0 comments
Labels
good first issue An issue that provides a good intro to working with the Myst codebase. Be helpful! spec An issue with relating to the specs around the Myst codebase (native or in-language).
Milestone

Comments

@faultyserver
Copy link
Member

This issue has been marked as a "Good First Issue"! If you'd like to tackle this issue as your first contribution to the project, be sure to read the Get Involved section of the README for some help with how to get started.

With the upgrade to Crystal 0.24.1 (see #53), expect_raises now requires an exception type to be specified by the callers. Out of haste to get 0.3.0 out, I've just put the general Exception class in for most cases where no argument was given. However, this is not a good practice, and the expected exceptions should be better restricted to more relevant types.

I think this improvement has two components: First, find all instances of expect_raises(Exception) in the spec suite and change them to more appropriate types (e.g., ParseError for errors raised by the parser, SyntaxError for errors from the Lexer, etc.). I don't think there are many of these.

Most instances probably won't be resolvable by the above fix, so the second part of this improvement would be to find all instances of raise in the Interpreter source code and turn them into appropriate RuntimeErrors. Not only does this help improve the spec, it also means that users can catch these errors in their programs, which is a good goal.

Adding a __raise_runtime_error helper method to util.cr also seems like a good idea to help mitigate the risk of this issue coming up again in the future.

Here's an example of how one of these improvements could be made. Line 18 here raises a native exception when re-assigning the value of a Constant:

when Const
if current_scope.has_key?(target.name)
raise "Re-assignment to constant value #{target.name}."
else
current_scope.assign(target.name, value)
end
.

Instead of a native exception being raised, I think this should raise a RuntimeError with the same message. Assuming the __raise_runtime_error method from above is implemented, it might look something like this:

# ...
when Const 
  if current_scope.has_key?(target.name) 
    __raise_runtime_error("Re-assignment to constant value #{target.name}.")
  else 
    current_scope.assign(target.name, value) 
  end
# ...

Then, in the specs, there's this test:

it "does not allow re-assignment to constants" do
error = expect_raises do
parse_and_interpret %q(
THING = 1
THING = 2
)
end

that should change to expect a RuntimeError instead of a normal Exception.

Hopefully that's enough direction to get started. As always, feel free to ask any questions either here or in the discord server. I don't think this issue requires too much knowledge of the interpreter, but it's hard for me to tell.

@faultyserver faultyserver added good first issue An issue that provides a good intro to working with the Myst codebase. Be helpful! spec An issue with relating to the specs around the Myst codebase (native or in-language). labels Dec 26, 2017
@faultyserver faultyserver added this to the Next milestone Dec 28, 2017
faultyserver added a commit to faultyserver/myst that referenced this issue Dec 30, 2017
…to RuntimeErrors, move nativelib specs into Myst.

This commit starts work on converting all errors raised by the interpreter into rescue-able RuntimeErrors. Specifically, this commit addresses all instances of `raise` in the Native Library, using the new `__raise_runtime_error` to raise an appropriate message.

When myst-lang#103 is addressed, these errors will change to proper error structures (rather than strings), but that is a future effort.

Some other instances of `raise` were also converted in the process of this commit.
@faultyserver faultyserver changed the title Improve exception types used for expect_raises. Raise only RuntimeErrors from the Interpreter Jan 1, 2018
@faultyserver faultyserver changed the title Raise only RuntimeErrors from the Interpreter Only raise RuntimeErrors from the Interpreter Jan 1, 2018
faultyserver added a commit to faultyserver/myst that referenced this issue Jan 1, 2018
By making MatchError a descendant of RuntimeError, it can be rescued inside the language itself. This is one more step to making the interpreter only raise RuntimeErrors.

The matcher specs needed a fair bit of reworking to play nicely with this change, including a change to `Interpreter#run` that tells the interpreter to allow errors to propogate farther up and be expected by the specs.
faultyserver added a commit to faultyserver/myst that referenced this issue Jan 6, 2018
…rom `Scope#[]`.

It's a bit of a cop-out solution, but the idea is that `IndexError`s raised by `Scope#[]` should not happen anywhere in the interpreter. Any case where the key may not be present should be guarded against appropriately. As such, if the `IndexError` propogates to outside of the Interpreter, it should be considered a bug to be fixed, as the messasge of the error now indicates.
faultyserver added a commit to faultyserver/myst that referenced this issue Jan 6, 2018
…_literal` raise case.

Any attempt to create a Value from a Node should guard that the Node is a Literal type. Attempting to create a Value from a non-literal type should be considered an Interpreter bug and addressed as such.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue An issue that provides a good intro to working with the Myst codebase. Be helpful! spec An issue with relating to the specs around the Myst codebase (native or in-language).
Projects
None yet
Development

No branches or pull requests

1 participant