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

Deprecate erlang:get_stacktrace/1 #1783

Merged
merged 4 commits into from
Apr 20, 2018

Conversation

bjorng
Copy link
Contributor

@bjorng bjorng commented Apr 16, 2018

#1634 extended the try/catch syntax to retrieve the stacktrace.

This pull request teaches the compiler to emit deprecation warnings for calls to erlang:get_stacktrace/0.

Also eliminate use of erlang:get_stacktrace/0 in code generated
by the ASN.1 compiler.
@bjorng bjorng added team:VM Assigned to OTP team VM feature testing currently being tested, tag is used by OTP internal CI labels Apr 16, 2018
@bjorng bjorng self-assigned this Apr 16, 2018
Those warnings don't make sense any more since erlang:get_stacktrace/0
is now deprecated.
@bjorng bjorng force-pushed the bjorn/deprecate-get_stacktrace/OTP-14692 branch from 6286380 to ce550c9 Compare April 16, 2018 12:33
@michalmuskala
Copy link
Contributor

Isn't it a bit too premature to deprecate erlang:get_stacktrace/0 on the same release that introduces the alternative?
This means it will be basically impossible to build warning-free libraries that support both OTP 20 and 21.

@ferd
Copy link
Contributor

ferd commented Apr 16, 2018

Not sure I agree with the concern here, or rather, share it in the same way.

First of all, I agree that this will make it pretty damn difficult for people to build patches that work on both on the day of the release, and you have to usually plan for a few days of adaptation while people communicate their workarounds -- which usually include optional compilation macros for each of them.

For everyone, one option is to use the nowarn_deprecated_function or {nowarn_deprecated_function, MFA} compiler options to make it through for the release days until something works.

Overall the one thing this really forces is either for developers to all make libraries that support multiple versions of Erlang's compiler through macro-heavy use, and/or users to all have multiple Erlang/OTP versions installed in order to do their work.

I'm happy to do that for the strings module (unicode support is important), but the stacktrace optimization is kind of a pain and I could imagine myself reducing the amount of debug information reported through logs to users in order to make compilation easier. I'm curious to see if the side-effect on Erlang operability will just find itself diminished by this change. What are the speed/safety benefits of this patch otherwise?


Aside

I'm also pretty bummed out at having to now go back through all my code (and the code in rebar3 is a particular pain point since we support 5 major OTP versions) and rework all of the try...catches by duplicating the functions that contain them (since we can't macro within a function body) just to handle the stacktrace portion.

This, on the other hand, means that anyone with some backwards or forwards compat concerns that go larger than the OTP team will share that problem whether or not the deprecation warnings are kept for now or saved for later. I think there is just not a great cultural norm for this instilled in the Erlang community, and every change (see the string module) just seems to hit everyone by surprise forever.

I also have a backlog of like 15 changes of this kind to backport to LYSE and I have no time for at this point, and the accelerating pace of breaking changes does make it a lot harder to just not give up and force a rewrite, which I do not have a few years for, but yeah.


@fenollp
Copy link
Contributor

fenollp commented Apr 16, 2018

Aside

Sorry to divert the conversation a bit further: would it be possible for you @ferd to publish LYSE's listings on github/gitlab, set up a CI & describe how people can contribute? I am sure people would be interested in contributing / discussing the contents of your (fantastic!) book(s)!

@ferd
Copy link
Contributor

ferd commented Apr 16, 2018

Not really, because that gets to be tricky with the shared/Joint copyright with a publisher already.

@bjorng
Copy link
Contributor Author

bjorng commented Apr 18, 2018

@michalmuskala Starting in OTP 20.1, there will not be a warning if nowarn_deprecated_function mentions an MFA for a function that is not deprecated. So the following line will not generate any warning in OTP 20.1 or higher:

 -compile([{nowarn_deprecated_function,{erlang,get_stacktrace,0}}]).

@bjorng
Copy link
Contributor Author

bjorng commented Apr 18, 2018

@ferd Perhaps a helper function could reduce the amount of duplicated code? For example:

try_catch(Expr, Handler) ->
    try
        Expr()
    catch
        C:E:S ->
            Handler(C, E, S)
    end.

There would still be a lot of code to change, but you would only have to provide two versions of try_catch/2.

@ferd
Copy link
Contributor

ferd commented Apr 18, 2018

This workaround ignores the ability of using try Exp of ... catch constructs, which can't really be dealt with in that way.

@bjorng bjorng merged commit 0eda677 into erlang:master Apr 20, 2018
@bjorng bjorng deleted the bjorn/deprecate-get_stacktrace/OTP-14692 branch April 20, 2018 12:06
@kostis
Copy link
Contributor

kostis commented Apr 20, 2018

I cannot build the master branch anymore after this has been committed,
My build, using make clean ; ./otp_build autoconf ; ./configure ; make fails with:

...
ERLC   ../ebin/xref_parser.beam
compile: warnings being treated as errors
/home/kostis/otp/bootstrap/lib/parsetools/include/yeccpre.hrl:60: erlang:get_stacktrace/0: deprecated; use the new try/catch syntax for retrieving the stack backtrace
/home/kostis/otp/make/aarch64-unknown-linux-gnu/otp.mk:124: recipe for target '../ebin/xref_parser.beam' failed
make[3]: *** [../ebin/xref_parser.beam] Error 1
make[3]: Leaving directory '/home/kostis/otp/lib/tools/src'

Is it just me?

@garazdawi
Copy link
Contributor

Relying on make clean to do a full clean is fragile at best. Did you try it with a fresh repo?

@kostis
Copy link
Contributor

kostis commented Apr 22, 2018

Relying on make clean to do a full clean is fragile at best.

I had to manually remove the (generated) file lib/tools/src/xref_parser.erl and then make worked.

A working, or at least better working, make clean would be appreciated.

But I think that the root of the problem is that there are missing dependencies from the various Makefiles. xref_parser.erl should have been rebuilt due to changes to lib/parsetools/include/yeccpre.hrl three months ago! -- on my system, it was not rebuilt then.

@ferd
Copy link
Contributor

ferd commented May 3, 2018

Here's sample code of what working around the deprecation means: https://github.com/erlang/rebar3/pull/1773/files

@okeuday
Copy link
Contributor

okeuday commented May 3, 2018

@ferd I am using a macro which makes it less painful:

% for features specific to Erlang/OTP version 20.x (and later versions)
-ifdef(ERLANG_OTP_VERSION_20).
-else.
-define(ERLANG_OTP_VERSION_21_FEATURES, true).
-endif.
% Get the stacktrace in a way that is backwards compatible
-ifdef(ERLANG_OTP_VERSION_21_FEATURES).
-define(STACKTRACE(ErrorType, Error, ErrorStackTrace),
        ErrorType:Error:ErrorStackTrace ->).
-else.
-define(STACKTRACE(ErrorType, Error, ErrorStackTrace),
        ErrorType:Error ->
            ErrorStackTrace = erlang:get_stacktrace(),).
-endif.

That allows use like:

try function1(Arg1)
catch
    ?STACKTRACE(exit, badarg, ErrorStackTrace)
        % do stuff with ErrorStackTrace
        % ...
end,

@fenollp
Copy link
Contributor

fenollp commented May 3, 2018

I actually have a third solution: https://github.com/fenollp/otp_vsn#on-st-matching :)

@ferd
Copy link
Contributor

ferd commented May 3, 2018

@okeuday yeah I might revisit to use that pattern. I don’t like that all my code requires a big macro like that everywhere, but the reduction in code duplication is worth it

ferd added a commit to ferd/rebar3 that referenced this pull request May 3, 2018
ferd added a commit to ferd/rebar3 that referenced this pull request May 3, 2018
@tomas-abrahamsson
Copy link
Contributor

It would have been useful if the ?OTP_RELEASE (plus -if and -elif) of eep-0044 would make it into some release. Then it would have been possible in a source file, using only the preprocessor, to support different OTP versions. To do that today, one also needs external support such as a build system. The source file cannot support different OTP versions on its own.

@okeuday
Copy link
Contributor

okeuday commented May 7, 2018

@tomas-abrahamsson I agree, though I like the name ERLANG_OTP_VERSION instead of OTP_RELEASE. There might be a small bit of ambiguity in EEP-0044 because it doesn't seem to clearly state where the version is coming from and it needs to be the version of the installation that has the compiler, not necessarily the version that compiled the compiler (so that may have been a concern that prevented its implementation?).

@kostis
Copy link
Contributor

kostis commented May 7, 2018

On a related issue. What will erlang:system_info(version) return for Erlang/OTP 21.0 ?
I guess not 9.3.1 as the current value is...

@bjorng
Copy link
Contributor Author

bjorng commented May 7, 2018

On a related issue. What will erlang:system_info(version) return for Erlang/OTP 21.0 ?

It will return 10.0.

@kostis
Copy link
Contributor

kostis commented May 7, 2018

Thanks for the quick reply. It will be good if this change happens soon (i.e. now) so that applications that rely on this built-in for checking for OTP changes and/or incompatibilities (e.g. PropEr) can test 'master'/RC2 and adapt their code bases appropriately before 21.0 is released.

@bjorng
Copy link
Contributor Author

bjorng commented May 7, 2018

If you checkout the tag itself and build from that the version numbers are correct. That is:

git checkout OTP-21.0-rc1

(If you'll need corrections not included in RC1, you could cherry-pick them.)

uwiger pushed a commit to aeternity/rebar3 that referenced this pull request Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants