Profile compiler option precedence appears to be backward

[tburghart]

So far, this is based on preliminary investigation, but if the docs are right (and they appear to be) then merging erl_opts from one or more profiles will invert the intended precedence of compiler options.

Current behaviour

According to the docs, which conveniently use erl_opts as an example, the options from profiles are prepended to the list of default options. However, these options eventually make their way to erl_lint:bool_option/4, where later elements in the list take precedence over earlier ones.

Expected behaviour

The intuitive behavior (to me, anyway) would be for the erl_opts in the profile(s) to take precedence over those in the default configuration, which means they would be appended, not prepended, to the options list.

[ferd]

Ugh. We had the same issue with Relx.

Am I right in thinking that if multiple conflicting ops are passed today (like [{d, 'MACRO', 1}, {d, 'MACRO', 2}], then 'MACRO' will have a value of 2 in the modules?

If that's expected we'll be in a bit of a pickle there since the profile merging algorithm does everything automatically. If we want to maintain that order we'll need to modify things there to special case erl_opts. If we can allow to swap that order, we can just reverse the merged list at the time we pass them to the compiler

[tburghart]

No, that will give you an error from the compiler: redefining macro 'MACRO'

[tburghart]

@ferd It gets worse.

Minimal test repo here: https://github.com/tburghart/rebar3_issue_1397

# rebar3 as p1,p2 shell
1> r3t:o().
erl_opts: [debug_info,
           {d,profile_p2},
           {d,profile_p1},
           {d,profile_default},
           nowarn_export_all,warn_export_all]

# rebar3 as p2,p1 shell
1> r3t:o().
erl_opts: [debug_info,
           {d,profile_p1},
           {d,profile_p2},
           {d,profile_default},
           nowarn_export_all,warn_export_all]

Note that the warn/nowarn options end up in the same order regardless of the profile order.

[ferd]

That's expected, sadly. We have to play with ordering in order to be able to do some level of deduplication. We have a custom algorithm that does a stable sort + merge based on profile order: https://github.com/erlang/rebar3/blob/master/src/rebar_utils.erl#L258-L369

The keys and values are sorted in the proper order and then deduplicated, or at least kept in an order such that a read through proplists or lists:keyfind (or a conversion to a dict) would maintain the entries as identical. The problem is that rather than having {warn_export_all, true | false}, the compiler has two distinct keys for this one. A generic algorithm cannot special case these no matter what we do.

[tburghart]

yeah, I looked through that code, and it seems you could do an order-preserving de-dup throughout, a la https://github.com/erlang/rebar3/blob/master/src/rebar_utils.erl#L296

Not preserving precedence gets quite problematic. I haven't looked into whether there are alternate approaches with overrides, but I'm trying to build up a common pattern for profiles throughout the 50+ repos that go into our product and really don't want to have to specify the full set of options in each profile with no defaults.

[ferd]

Yeah. We couldn't handle nowarn_export_all or warn_export_all though, and some option lists have the ability of using multiple duplicate keys for various values (like macro definition). It's therefore not really possible for us to properly do that dedup safely across all contexts unless the entire entry is identical, but the proplist order generally worked well everywhere else.

[tburghart]

How would you feel about a PR that added knowledge of compiler option patterns to the profile merge? I think it'd just be limited to erl_opts now, because the other stuff like eunit_erl_opts is all gone in v3, right?

[ferd]

they're still there as legacy support for other apps and libs, along with ct_erl_opts.

[tburghart]

That would complicate it marginally, but the patterns are all the same so it wouldn't be much more so.

[tburghart]

@ferd The following looks to me like appropriate logic specific to compiler options, where merge_opt/3 is a function head inserted at https://github.com/erlang/rebar3/blob/master/src/rebar_opts.erl#L120

There may be a more efficient way to code it, but I think the behavior's correct - do you agree?

merge_opt(erl_opts, NewValue, OldValue) ->
    merge_erl_opts(lists:reverse(OldValue), lists:reverse(NewValue));

merge_erl_opts([Opt | Opts], []) ->
    merge_erl_opts(Opts, [Opt]);
merge_erl_opts([Opt | Opts], Merged) ->
    case lists:member(Opt, Merged) of
        true ->
            merge_erl_opts(Opts, Merged);
        _ ->
            case is_atom(Opt) of
                true ->
                    Str = atom_to_list(Opt),
                    case Str of
                        "warn_" ++ _ ->
                            NOpt = list_to_atom("no" ++ Str),
                            case lists:member(NOpt, Merged) of
                                true ->
                                    merge_erl_opts(Opts, Merged);
                                _ ->
                                    merge_erl_opts(Opts, [Opt | Merged])
                            end;
                        "nowarn_" ++ _ ->
                            WOpt = list_to_atom(lists:nthtail(2, Str)),
                            case lists:member(WOpt, Merged) of
                                true ->
                                    merge_erl_opts(Opts, Merged);
                                _ ->
                                    merge_erl_opts(Opts, [Opt | Merged])
                            end;
                        _ ->
                            merge_erl_opts(Opts, [Opt | Merged])
                    end;
                _ ->
                    merge_erl_opts(Opts, [Opt | Merged])
            end
    end;
merge_erl_opts([], Merged) ->
    lists:reverse(Merged).

[ferd]

I can't visually asses the correctness of code like that at a glance. I'd probably need tests or samples to run it from. That's fairly complex code.

[tburghart]

I've tweaked it a little (too many list reversals) and integrated it so I could test it, gonna do a PR soon.

[tburghart]

Once I got the options in the right order a whole bunch of that code became extraneous and the patch is pretty simple.