Skip to content

Commit

Permalink
Merge branch 'bjorn/kernel/undefined-function-handler/OTP-10617'
Browse files Browse the repository at this point in the history 
* bjorn/kernel/undefined-function-handler/OTP-10617:
  Teach error_handler to call '$handle_undefined_function'
  • Loading branch information
@bjorng
bjorng committed Jan 18, 2013
2 parents 35adf88 + f22da57 commit 209a479
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 41 deletions.
38 changes: 29 additions & 9 deletions lib/kernel/doc/src/error_handler.xml
View file
Expand Up @@ -43,19 +43,39 @@
A (possibly empty) list of arguments <c>Arg1,..,ArgN</c>
</type_desc>
<desc>
<p>This function is evaluated if a call is made to
<p>This function is called by the run-time system if a call is made to
<c><anno>Module</anno>:<anno>Function</anno>(Arg1,.., ArgN)</c> and
<c><anno>Module</anno>:<anno>Function</anno>/N</c> is undefined. Note that
<c>undefined_function/3</c> is evaluated inside the process
making the original call.</p>
<p>If <c><anno>Module</anno></c> is interpreted, the interpreter is invoked
and the return value of the interpreted
<c><anno>Function</anno>(Arg1,.., ArgN)</c> call is returned.</p>
<p>Otherwise, it returns, if possible, the value of
<c>apply(<anno>Module</anno>, <anno>Function</anno>, <anno>Args</anno>)</c> after an attempt has been
made to autoload <c><anno>Module</anno></c>. If this is not possible, the
call to <c><anno>Module</anno>:<anno>Function</anno>(Arg1,.., ArgN)</c> fails with
exit reason <c>undef</c>.</p>

<p>This function will first attempt to autoload
<c><anno>Module</anno></c>. If that is not possible,
an <c>undef</c> exception will be raised.</p>

<p>If it was possible to load <c><anno>Module</anno></c>
and the function <c><anno>Function</anno>/N</c> is exported,
it will be called.</p>

<p>Otherwise, if the function <c>'$handle_undefined_function'/2</c>
is exported, it will be called as
<c>'$handle_undefined_function'(</c><anno>Function</anno>,
<anno>Args</anno>).
</p>
<p>Otherwise an <c>undef</c> exception will be raised.</p>
</desc>
</func>
<func>
<name name="raise_undef_exception" arity="3"/>
<fsummary>Raise an undef exception</fsummary>
<type_desc variable="Args">
A (possibly empty) list of arguments <c>Arg1,..,ArgN</c>
</type_desc>
<desc>
<p>Raise an <c>undef</c> exception with a stacktrace indicating
that <c><anno>Module</anno>:<anno>Function</anno>/N</c> is
undefined.
</p>
</desc>
</func>
<func>
Expand Down
53 changes: 21 additions & 32 deletions lib/kernel/src/error_handler.erl
View file
Expand Up @@ -23,10 +23,12 @@
%% "error_handler: add no_native compiler directive"
-compile(no_native).

%% A simple error handler.
%% Callbacks called from the run-time system.
-export([undefined_function/3,undefined_lambda/3,breakpoint/3]).

-export([undefined_function/3, undefined_lambda/3, stub_function/3,
breakpoint/3]).
%% Exported utility functions.
-export([raise_undef_exception/3]).
-export([stub_function/3]).

-spec undefined_function(Module, Function, Args) ->
any() when
Expand All @@ -41,12 +43,7 @@ undefined_function(Module, Func, Args) ->
true ->
apply(Module, Func, Args);
false ->
case check_inheritance(Module, Args) of
{value, Base, Args1} ->
apply(Base, Func, Args1);
none ->
crash(Module, Func, Args)
end
call_undefined_function_handler(Module, Func, Args)
end;
{module, _} ->
crash(Module, Func, Args);
Expand Down Expand Up @@ -77,6 +74,14 @@ undefined_lambda(Module, Fun, Args) ->
breakpoint(Module, Func, Args) ->
(int()):eval(Module, Func, Args).

-spec raise_undef_exception(Module, Function, Args) -> no_return() when
Module :: atom(),
Function :: atom(),
Args :: list().

raise_undef_exception(Module, Func, Args) ->
crash({Module,Func,Args,[]}).

%% Used to make the call to the 'int' module a "weak" one, to avoid
%% building strong components in xref or dialyzer.

Expand Down Expand Up @@ -130,27 +135,11 @@ ensure_loaded(Module) ->
stub_function(Mod, Func, Args) ->
exit({undef,[{Mod,Func,Args,[]}]}).

check_inheritance(Module, Args) ->
Attrs = erlang:get_module_info(Module, attributes),
case lists:keyfind(extends, 1, Attrs) of
{extends, [Base]} when is_atom(Base), Base =/= Module ->
%% This is just a heuristic for detecting abstract modules
%% with inheritance so they can be handled; it would be
%% much better to do it in the emulator runtime
case lists:keyfind(abstract, 1, Attrs) of
{abstract, [true]} ->
case lists:reverse(Args) of
[M|Rs] when tuple_size(M) > 1,
element(1,M) =:= Module,
tuple_size(element(2,M)) > 0,
is_atom(element(1,element(2,M))) ->
{value, Base, lists:reverse(Rs, [element(2,M)])};
_ ->
{value, Base, Args}
end;
_ ->
{value, Base, Args}
end;
_ ->
none
call_undefined_function_handler(Module, Func, Args) ->
Handler = '$handle_undefined_function',
case erlang:function_exported(Module, Handler, 2) of
false ->
crash(Module, Func, Args);
true ->
Module:Handler(Func, Args)
end.
1 change: 1 addition & 0 deletions lib/kernel/test/Makefile
View file
Expand Up @@ -48,6 +48,7 @@ MODULES= \
erl_distribution_SUITE \
erl_distribution_wb_SUITE \
erl_prim_loader_SUITE \
error_handler_SUITE \
error_logger_SUITE \
error_logger_warn_SUITE \
file_SUITE \
Expand Down
68 changes: 68 additions & 0 deletions lib/kernel/test/error_handler_SUITE.erl
View file
@@ -0,0 +1,68 @@
%%
%% %CopyrightBegin%
%%
%% Copyright Ericsson AB 2013. All Rights Reserved.
%%
%% The contents of this file are subject to the Erlang Public License,
%% Version 1.1, (the "License"); you may not use this file except in
%% compliance with the License. You should have received a copy of the
%% Erlang Public License along with this software. If not, it can be
%% retrieved online at http://www.erlang.org/.
%%
%% Software distributed under the License is distributed on an "AS IS"
%% basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See
%% the License for the specific language governing rights and limitations
%% under the License.
%%
%% %CopyrightEnd%
%%
-module(error_handler_SUITE).

-export([all/0,suite/0,groups/0,init_per_suite/1,end_per_suite/1,
init_per_group/2,end_per_group/2,
undefined_function_handler/1]).

%% Callback from error_handler.
-export(['$handle_undefined_function'/2]).

suite() -> [{ct_hooks,[ts_install_cth]}].

all() ->
[undefined_function_handler].

groups() ->
[].

init_per_suite(Config) ->
Config.

end_per_suite(_Config) ->
ok.

init_per_group(_GroupName, Config) ->
Config.

end_per_group(_GroupName, Config) ->
Config.


%%-----------------------------------------------------------------

undefined_function_handler(_) ->
42 = ?MODULE:forty_two(),
42 = (id(?MODULE)):forty_two(),
{ok,{a,b,c}} = ?MODULE:one_arg({a,b,c}),
{ok,{a,b,c}} = (id(?MODULE)):one_arg({a,b,c}),
{'EXIT',{undef,[{?MODULE,undef_and_not_handled,[[1,2,3]],[]}|_]}} =
(catch ?MODULE:undef_and_not_handled([1,2,3])),
ok.

'$handle_undefined_function'(forty_two, []) ->
42;
'$handle_undefined_function'(one_arg, [Arg]) ->
{ok,Arg};
'$handle_undefined_function'(Func, Args) ->
error_handler:raise_undef_exception(?MODULE, Func, Args).

id(I) ->
I.

15 comments on commit 209a479

@DeadZen
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reconsider this functionality, it will not be used for good in the long run.
Its usage applies to at best 1 out of 100 modules, and creates similar issues found with parameterized modules.
It exposes internals with regard to the raise_undef_exception while the dynamic function calling will make finding errors more difficult
and to put it plainly the error handler should not be calling functions in user modules.

If one calls an undefined function, it should just crash and not try to "handle it" as doing so would violate Erlang semantics.

This feature will simply scratch an itch and become a rash. -1

@dieswaytoofast
Copy link

Choose a reason for hiding this comment

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

I'm with DeadZen on this - from the perspective of both predictability and security, this gives me the willies.
If you must go down this road, I'd prefer that it be far more constrained (handlers can only go to certain types/names of modules, special compiler flags must be invoked, secret signs flashed, etc.).
That said, I still see this as potentially causing issues down the road...
-1

@andrzejsliwa
Copy link

Choose a reason for hiding this comment

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

no method_missing for erlang. This will be overused - 1

@evanmiller
Copy link
Contributor

Choose a reason for hiding this comment

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

Three cheers for $handle_undefined_function. This will make code generation in BossDB a million times easier. Who cares if you think it will be overused in other people's projects? It makes my life easier and doesn't affect yours. Mind your own business!

+1

@bullno1
Copy link

Choose a reason for hiding this comment

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

Another -1 from me. This makes room for many unpredictable behaviours. For those who wants this behaviour: how hard is it to just create your own apply_with_handler(M, F, A)? Making error_handler calling functions is a terrible hack.

@yrashk
Copy link

@yrashk yrashk commented on 209a479 Jan 20, 2013

Choose a reason for hiding this comment

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

This doesn't actually "makes room" — this behaviour has been available in a slightly more complex way for ages

@nox
Copy link
Contributor

@nox nox commented on 209a479 Jan 20, 2013

Choose a reason for hiding this comment

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

@evanmiller It affects mine because now if I use a library or an application that itself uses that, I'm not even sure the code will fail properly if I make a typo in a function name. And as this feature isn't supported by Dialyzer nor xref (and can't be), I will have no way to know whether I did wrote the function name badly, because the tool will always bark at me, whether the call is legit or not.

@yrashk
Copy link

@yrashk yrashk commented on 209a479 Jan 20, 2013

Choose a reason for hiding this comment

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

@nox, following this logic, apply should be removed, too.

Also, nobody forces you to use libraries you don't like :)

@ferd
Copy link
Contributor

@ferd ferd commented on 209a479 Jan 20, 2013

Choose a reason for hiding this comment

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

As long as the call order is well-defined to always be the same (process-defined handler -> module-defined handler), there should be no big consistency problem.

There should be work done in order to make sure that a crash in the '$handle_undefined_function' handler does not change the error type from undef to whatever the crash was.

In my opinion, this will allow to keep a more declarative type of error handling there while keeping the caller's expectations sane, and will put the burden of shoddy programming maintenance on the authors of the original handler, and not push it to the user of the library.

Let's imagine I use a module that used this feature, gets updated, and has a bug that breaks the functionality within '$handle_undefined_function'. As the user of that module, I may get anything from undef to whatever the handler uses and can generate (badarg, badarith, badfun, etc.). This can be very confusing to debug and manage.

On the other hand, if the error is restricted to undef, the unknown behaviour is pushed back on the maintainer of the library to figure out, and inspecting the module will show that the function is indeed missing. It breaks fewer expectations.

I would also be in favor of a new error type of the kind module_author_uses_features_he_or_she_is_not_understanding_please_blame_them to be returned to make the problem 100% obvious and embarrassing. I'm barely kidding here.

I'm not against the idea, but -1 with the current implementation.

@essen
Copy link
Contributor

@essen essen commented on 209a479 Jan 20, 2013

Choose a reason for hiding this comment

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

I like the feature. I hate the implementation. Here's a proposal for an hopefully better implementation of this feature, in a few short steps:

  • User adds an attribute to his module to declare which functions are defined through the undefined function handler
  • User writes the undefined function handler and handles all these functions, no more, no less
  • If the function is undefined and not listed in the attribute, then we fail with an undef directly
  • If the function is undefined and is listed in the attribute, we call the handler. If the handler crashes, we propagate the crash as expected (this allows us to pattern match the arguments in the handler safely and still get a badarg as expected)
  • The user does not have to handle undef anymore. We protect him from writing code that breaks expectations by forcing him to declare which functions calls are actually valid

@yrashk
Copy link

@yrashk yrashk commented on 209a479 Jan 20, 2013

Choose a reason for hiding this comment

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

User adds an attribute to his module to declare which functions are defined through the undefined function handler

I think the whole idea people use it for is that they don't know those function names ahead of time

@essen
Copy link
Contributor

@essen essen commented on 209a479 Jan 20, 2013

Choose a reason for hiding this comment

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

When you -extend you know what functions you don't implement. When you write functionality that has to go through the undefined handler you know which functionality do. In what case do you not know what goes through it?

@yrashk
Copy link

@yrashk yrashk commented on 209a479 Jan 20, 2013 via email

Choose a reason for hiding this comment

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

@essen
Copy link
Contributor

@essen essen commented on 209a479 Jan 20, 2013

Choose a reason for hiding this comment

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

Evan is generating code. I'm sure if he can generate code for calling first_name then he can add an attribute listing it. He currently does it for exports, right? What's the difference?

@psyeugenic
Copy link
Contributor

Choose a reason for hiding this comment

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

Please discuss issue at mailing lists. GitHub is fine for commenting but not for discussing.

Please sign in to comment.