-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
add null check for module-default-defs #35363
Conversation
Great, thanks! Could you add a test case to test/syntax.jl? |
5a0b565 adds a fix for #35367 as well Output for those two lines in the REPL:
|
src/ast.c
Outdated
@@ -575,7 +575,7 @@ static jl_value_t *scm_to_julia_(fl_context_t *fl_ctx, value_t e, jl_module_t *m | |||
assert(jl_is_symbol(ex)); | |||
temp = jl_module_globalref(jl_core_module, (jl_sym_t*)ex); | |||
} | |||
else if (sym == inert_sym || (sym == quote_sym && (!iscons(car_(e))))) { | |||
else if ((sym == inert_sym || (sym == quote_sym && (!iscons(car_(e))))) && iscons(e)) { |
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.
iscons(e)
needs to be checked before car_(e)
; car
is only valid if e
is a cons.
test/core.jl
Outdated
eval(Expr(:call, :eval, Expr(:quote, Expr(:module, true, :bar, Expr(:block))))) | ||
eval(Expr(:module, true, :bar, Expr(:block))) | ||
eval(Expr(:quote, Expr(:module, true, :bar, Expr(:quote)))) | ||
eval(Expr(:call, :eval, Expr(:quote, Expr(:module, true, :bar, Expr(:quote))))) |
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.
I don't think this should be valid; the 3rd argument to module
has to be a block
expression. That can be checked at the top of jl_eval_module_expr
.
@JeffBezanson added an if check inside The following now throw an Exception:
The following does not return an ErrorException (but no longer segfaults ):
I think due to the quoting? Which I think is fine? |
I just hit this bug myself — glad to see a fix already here! My use-case was a macro: julia> macro foo()
Expr(:module, true, esc(:Bar), Expr(:block))
end
@foo (macro with 1 method)
julia> @foo
WARNING: replacing module Bar.
signal (11): Segmentation fault: 11 |
Right; the |
add null check for module-default-defs, fix JuliaLang#34544 fix JuliaLang#35367 as well change jl_eval_module_expr to check 3rd arg is block
Description
Issue #34544.
In PR #35337 there was a suggestion to add a check for
body
inmodule-default-defs
insrc/jlfrontend.scm
. This adds that check.Testing
In the julia repl I tried a couple more variations that would cause a segfault