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.

Sign up for GitHub

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

Jump to bottom

Remove exception catching for __init__ functions, add InitError exception type #12576

Merged
merged 2 commits into from
Aug 18, 2015
Merged

Remove exception catching for __init__ functions, add InitError exception type #12576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b5ef61
Added InitError exception to wrap errors from __init__
jdlangs Aug 13, 2015
5898d2e
Add WrappedException abstract type
jdlangs Aug 14, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions base/base.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,6 @@ type KeyError <: Exception
key
end

type LoadError <: Exception
file::AbstractString
line::Int
error
end

type MethodError <: Exception
f
args
Expand All @@ -59,6 +53,21 @@ type AssertionError <: Exception
AssertionError(msg) = new(msg)
end

#Generic wrapping of arbitrary exceptions
#Subtypes should put the exception in an 'error' field
abstract WrappedException <: Exception

type LoadError <: WrappedException
file::AbstractString
line::Int
error
end

type InitError <: WrappedException
mod::Symbol
error
end

ccall(:jl_get_system_hooks, Void, ())


Expand Down
10 changes: 10 additions & 0 deletions base/docs/helpdb.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13646,6 +13646,16 @@ An error occurred while `including`, `requiring`, or `using` a file. The error s
"""
LoadError

doc"""
```rst
::
InitError(mod::Symbol, error)

An error occurred when running a module's `__init__` function. The actual error thrown is available in the `.error` field.
```
"""
InitError

doc"""
```rst
::
Expand Down
1 change: 1 addition & 0 deletions base/exports.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ export
InvalidStateException,
KeyError,
LoadError,
InitError,
MethodError,
NullException,
ParseError,
Expand Down
2 changes: 1 addition & 1 deletion base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ function show(io::IO, ex::PrecompilableError)
end
end
precompilableerror(ex::PrecompilableError, c) = ex.isprecompilable == c
precompilableerror(ex::LoadError, c) = precompilableerror(ex.error, c)
precompilableerror(ex::WrappedException, c) = precompilableerror(ex.error, c)
precompilableerror(ex, c) = false
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is useful for other cases, but this change kind of begs for abstract WrappedException <: Exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had thought about it, but it did seem like a pretty small corner case. I suppose there's no harm in future-proofing though.


# put at the top of a file to force it to be precompiled (true), or
Expand Down
7 changes: 7 additions & 0 deletions base/replutil.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,13 @@ function showerror(io::IO, ex::LoadError, bt; backtrace=true)
end
showerror(io::IO, ex::LoadError) = showerror(io, ex, [])

function showerror(io::IO, ex::InitError, bt; backtrace=true)
print(io, "InitError: ")
showerror(io, ex.error, bt, backtrace=backtrace)
print(io, "\nduring initialization of module $(ex.mod)")
Copy link
Member

Choose a reason for hiding this comment

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

I feel this should be:

showerror(io::IO, ex::InitError) = print(io, "InitError: during initialization of module $(ex.mod"))

the backtrace will already be appended

Copy link
Member

Choose a reason for hiding this comment

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

@jakebolewski, It seems like @phobon's code is correct; it should be analogous to showerror(io::IO, ex::LoadError), and you need to show ex.error at minimum, and you need to pass the backtrace through in case ex.error has a special backtrace handler.

end
showerror(io::IO, ex::InitError) = showerror(io, ex, [])

function showerror(io::IO, ex::DomainError, bt; backtrace=true)
print(io, "DomainError:")
for b in bt
Expand Down
2 changes: 2 additions & 0 deletions doc/manual/control-flow.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,8 @@ built-in :exc:`Exception`\ s listed below all interrupt the normal flow of contr
+------------------------------+
| :exc:`InexactError` |
+------------------------------+
| :exc:`InitError` |
+------------------------------+
| :exc:`InterruptException` |
+------------------------------+
| :exc:`InvalidStateException` |
Expand Down
1 change: 1 addition & 0 deletions src/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ jl_datatype_t *jl_argumenterror_type;
jl_datatype_t *jl_typeerror_type;
jl_datatype_t *jl_methoderror_type;
jl_datatype_t *jl_loaderror_type;
jl_datatype_t *jl_initerror_type;
jl_datatype_t *jl_undefvarerror_type;
jl_datatype_t *jl_ref_type;
jl_datatype_t *jl_pointer_type;
Expand Down
1 change: 1 addition & 0 deletions src/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,7 @@ DLLEXPORT void jl_get_system_hooks(void)
jl_typeerror_type = (jl_datatype_t*)basemod("TypeError");
jl_methoderror_type = (jl_datatype_t*)basemod("MethodError");
jl_loaderror_type = (jl_datatype_t*)basemod("LoadError");
jl_initerror_type = (jl_datatype_t*)basemod("InitError");
jl_complex_type = (jl_datatype_t*)basemod("Complex");
}

Expand Down
1 change: 1 addition & 0 deletions src/julia.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ extern DLLEXPORT jl_datatype_t *jl_utf8_string_type;
extern DLLEXPORT jl_datatype_t *jl_errorexception_type;
extern DLLEXPORT jl_datatype_t *jl_argumenterror_type;
extern DLLEXPORT jl_datatype_t *jl_loaderror_type;
extern DLLEXPORT jl_datatype_t *jl_initerror_type;
extern DLLEXPORT jl_datatype_t *jl_typeerror_type;
extern DLLEXPORT jl_datatype_t *jl_methoderror_type;
extern DLLEXPORT jl_datatype_t *jl_undefvarerror_type;
Expand Down
11 changes: 8 additions & 3 deletions src/module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,9 +542,14 @@ void jl_module_run_initializer(jl_module_t *m)
jl_apply(f, NULL, 0);
}
JL_CATCH {
jl_printf(JL_STDERR, "WARNING: error initializing module %s:\n", m->name->name);
jl_static_show(JL_STDERR, jl_exception_in_transit);
jl_printf(JL_STDERR, "\n");
if (jl_initerror_type == NULL) {
jl_rethrow();
}
else {
jl_rethrow_other(jl_new_struct(jl_initerror_type, m->name,
jl_exception_in_transit));
jl_printf(JL_STDERR, "\n");
Copy link
Member

Choose a reason for hiding this comment

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

It's really weird to print something if the exception isn't being handled. If you tried to trap and hide all of these exceptions, you would still get empty lines scrolling in stderr.

EDIT: I see this is after a rethrow so I guess it's just dead code.

}
}
}

Expand Down
26 changes: 19 additions & 7 deletions src/toplevel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ jl_value_t *jl_eval_module_expr(jl_expr_t *ex)
jl_typeerror_type = NULL;
jl_methoderror_type = NULL;
jl_loaderror_type = NULL;
jl_initerror_type = NULL;
jl_current_task->tls = jl_nothing; // may contain an entry for :SOURCE_FILE that is not valid in the new base
}
// export all modules from Main
Expand Down Expand Up @@ -205,12 +206,18 @@ jl_value_t *jl_eval_module_expr(jl_expr_t *ex)
arraylist_push(&module_stack, newm);

if (outermost == NULL || jl_current_module == jl_main_module) {
size_t i, l=module_stack.len;
for(i = stackidx; i < l; i++) {
jl_module_load_time_initialize((jl_module_t*)module_stack.items[i]);
JL_TRY {
size_t i, l=module_stack.len;
for(i = stackidx; i < l; i++) {
jl_module_load_time_initialize((jl_module_t*)module_stack.items[i]);
}
assert(module_stack.len == l);
module_stack.len = stackidx;
}
JL_CATCH {
module_stack.len = stackidx;
jl_rethrow();
}
assert(module_stack.len == l);
module_stack.len = stackidx;
}

return (jl_value_t*)newm;
Expand Down Expand Up @@ -585,8 +592,13 @@ jl_value_t *jl_parse_eval_all(const char *fname, size_t len)
jl_rethrow();
}
else {
jl_rethrow_other(jl_new_struct(jl_loaderror_type, fn, ln,
jl_exception_in_transit));
if(jl_typeis(jl_exception_in_transit, jl_initerror_type)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this makes sense. Whether an error causes loading a file to fail is orthogonal to the underlying cause of the error. For example if this is "x.jl":

using Foo  # throws InitError

Foo.f()

we won't get a LoadError for loading x.jl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I think I implicitly assumed this would only get used in one-file-one-module scenarios.

It seems the most appropriate thing would be to run the initializers only after all the code has been loaded, but I suppose that is quite a major change. Is a doubly wrapped exception what makes the most sense then?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just ditch InitError and use LoadError for __init__ errors? After all, a LoadError is what you would get from the same code pasted at the end of the module.

Copy link
Member

Choose a reason for hiding this comment

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

Just double-wrapping the exception is probably better. That way the decision to have InitError is orthogonal to how everything else works. I'm not sure what you can do by catching InitErrors, but at least they provide more information for printing exceptions, which seems harmless enough for now.

jl_rethrow();
}
else {
jl_rethrow_other(jl_new_struct(jl_loaderror_type, fn, ln,
jl_exception_in_transit));
}
}
}
jl_stop_parsing();
Expand Down
8 changes: 8 additions & 0 deletions test/core.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,14 @@ end
@test names(Module(:anonymous), true, true) != [:anonymous]
@test names(Module(:anonymous, false), true, true) == [:anonymous]

# exception from __init__()
@test_throws InitError include_string(
"""
module TestInitError
__init__() = error()
end
""")

# issue #7307
function test7307(a, ret)
try
Expand Down