-
Notifications
You must be signed in to change notification settings - Fork 139
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
Implement file parsing error handling #2365
Conversation
Oops, forgot to run all the tests. Will update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Looks like the windows tests are failing because it's a different errno error (I think that's expected). Can you check for a superclass? |
Ahh it looks like FFI is failing because it needs to be updated. You can check that with: PRISM_FFI_BACKEND=1 bundle exec rake |
I'm a little bit stuck. FFI tests are failing because we check Line 163 in 837af48
I can't figure out how (or even if) I can get the |
24f83c1
to
a736bef
Compare
After pairing with Kevin (thank you!) we've changed |
92642f2
to
b8cb050
Compare
FWIW there is |
b8cb050
to
7ee467a
Compare
test/prism/parse_test.rb
Outdated
error = assert_raise Errno::EFAULT do | ||
Prism.dump_file(nil) | ||
end | ||
|
||
assert_equal "Bad address", error.message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit strange that nil would raise EFAULT.
I would expect a coercion error, just like File.read
does:
$ ruby -e 'File.read nil'
-e:1:in `read': no implicit conversion of nil into String (TypeError)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now raising a TypeError
.
42d53f8
to
bb75713
Compare
test/prism/parse_test.rb
Outdated
end | ||
end | ||
|
||
def test_profile_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one you should remove, because it's not implemented in the FFI. We'll trust that it'll work since it's not supposed to be exposed anyway. But thank you for being thorough!
This PR implements proper file parsing error handling. Previously `file_options` would call `pm_string_mapped_init` which would print an error from `perror`. However this wouldn't raise a proper Ruby error so it was just a string output. I've done the following: - Raise an error from `rb_syserr_fail` with the filepath in `file_options`. - No longer return `Qnil` if `file_options` returns false (because now it will raise) - Update `file_options` to return `static void` instead of `static bool`. - Update `file_options` and `profile_file` to check the type so when passing `nil` we see a `TypeError`. - Delete `perror` from `pm_string_mapped_init` - Update `FFI` backend to raise appropriate errors when calling `pm_string_mapped_init`. - Add tests for `dump_file`, `lex_file`, `parse_file`, `parse_file_comments`, `parse_lex_file`, and `parse_file_success?` when a file doesn't exist and for `nil`. - Updates the `bin/parse` script to no longer raise it's own `ArgumentError` now that we raise a proper error. Fixes: ruby#2207
bb75713
to
b2f7494
Compare
Ok I think this is ready now! 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job!
Following changes made in ruby/prism#2365 this implements error handling for when `pm_string_mapped_init` returns `false`. Related: ruby/prism#2207
Following changes made in ruby/prism#2365 this implements error handling for when `pm_string_mapped_init` returns `false`. Related: ruby/prism#2207
Following changes made in ruby/prism#2365 this implements error handling for when `pm_string_mapped_init` returns `false`. Related: ruby/prism#2207
Implement file parsing error handling
This PR implements proper file parsing error handling. Previously
file_options
would callpm_string_mapped_init
which would print anerror from
perror
. However this wouldn't raise a proper Ruby error soit was just a string output. I've done the following:
rb_syserr_fail
with the filepath infile_options
.Qnil
iffile_options
returns false (because nowit will raise)
file_options
to returnstatic void
instead ofstatic bool
.file_options
andprofile_file
to check the type so whenpassing
nil
we see aTypeError
.perror
frompm_string_mapped_init
FFI
backend to raise appropriate errors when callingpm_string_mapped_init
.dump_file
,lex_file
,parse_file
,parse_file_comments
,parse_lex_file
, andparse_file_success?
,when a file doesn't exist and for
nil
.bin/parse
script to no longer raise it's ownArgumentError
now that we raise a proper error.Fixes: #2207