Parse transform can't handle `Attr' as a variable #54

Merged
merged 2 commits into from Feb 26, 2013

4 participants

@olle

Log statements with Attr bound to a variable, causes the parse transform to fail with a function_clause. To reproduce, try out the following:

Attr = [{a, alpha}, {b, beta}],
lager:info(Attr, "Hello ~s", ["world!"]).

Crashes during compile with something like the following (cropped).

{function_clause, [
  {lager_transform, concat_lists, [
    {var,24,'Attr'}, ...

The cause is clear, it is expected that the DefaultAttrs can be appended to the passed property list of attributes, so there's no clause for a var in concat_lists.

But - It's already noted in lager_transform.erl that transformation of logging statements is ambiguous for calls with arity 3 (2 arguments). Sadly I think it's not only that, but actually also broken for arity 2 if Attr is a variable - the match for a property list doesn't work. Actually, Is is at all possible to walk the AST in a parse transform, when the variable is passed from outside the module? To me it seems that this could be the real bringer of bad news.

I like the general idea of lager, having log statements transformed in order to add implicit meta-data, but I think that the way the API is designed right now, it's very hard to support a more free way of using, and abusing, it.

To be on the safe side I guess the API has to use stricter ordering for optional parameters i.e. attributes are always arity 3 and always on the end of the call. Another option could be to use tagged parameters.

Perhaps a solution for lager, and this API version, is to simply amend the documentation and more clearly state that the attributes, must be defined as an inline property list.

Or does someone else have a solution for this?

With kind regards
/O

@Vagabond

How does this look? Sorry it took forever to look into this...

@vegayours

Hi! Voting this too.
But I think we can do a little better, because Attrs can be not only variable, but a function call, maybe record field(why not?)
My solution is here vegayours#1
It is almost the same (I have found this pull request after fixing Attrs as variable issue)
If you are going to accept this pull request we can merge our work

@Vagabond

Yes, you can merge your work with mine, and add some tests to support function call arguments or whatever (list comprehensions, whatever).

@vegayours
Vagabond added some commits Dec 13, 2012
@Vagabond Vagabond Support variables as arguments to lager:info and friends
As long as *one* of the arguments is a literal, lager can figure out
what you're trying to do and (re)arrange the arguments as necessary.
95fdf93
@Vagabond Vagabond Also allow list comps, function calls and record fields as lager argu…
…ments
0ec0d90
@Vagabond

Parse transform now handles variables, list comprehensions, function calls and record fields. Anything else? Begin/End blocks seem kind of stupid to support, as do clauses/ifs.

@seancribbs

Nothing in this PR concerns me except these EQC failures on seemingly unrelated code. Can you confirm?

Exception: badarg
FmtStr: []
Args:   []
Failed! After 1 tests.
{[],3}
trunc_io_eqc:42: eqc_test_...*failed*
in function trunc_io_eqc:'-eqc_test_/0-fun-1-'/1 (test/trunc_io_eqc.erl, line 42)
**error:{assertEqual_failed,[{module,trunc_io_eqc},
                     {line,42},
                     {expression,"eqc : quickcheck ( eqc : testing_time ( 14 , ? QC_OUT ( prop_format ( ) ) ) )"},
                     {expected,true},
                     {value,false}]}
  output:<<"Starting Quviq QuickCheck version 1.27.7
   (compiled at {{2012,11,20},{14,59,9}})
Licence for Basho reserved until {{2013,2,26},{10,54,59}}
">>

Failed! Reason: 
{'EXIT',{badarg,[{lager_trunc_io,format,
                                 [[],[]],
                                 [{file,"src/lager_trunc_io.erl"},{line,66}]},
                 {trunc_io_eqc,'-prop_equivalence/0-fun-2-',1,
                               [{file,"test/trunc_io_eqc.erl"},{line,197}]}]}}
After 1 tests.
[]
trunc_io_eqc:43: eqc_test_...*failed*
in function trunc_io_eqc:'-eqc_test_/0-fun-4-'/1 (test/trunc_io_eqc.erl, line 43)
**error:{assertEqual_failed,[{module,trunc_io_eqc},
                     {line,43},
                     {expression,"eqc : quickcheck ( eqc : testing_time ( 14 , ? QC_OUT ( prop_equivalence ( ) ) ) )"},
                     {expected,true},
                     {value,false}]}
@seancribbs

It seems @engelsanchez brought this up on #108.

@Vagabond

I think this is a different EQC problem, caused by this change. I'll investigate.

@Vagabond

Oh, so that EQC failure looks to be fixed over on the iolist branch:

https://github.com/basho/lager/pull/108/files#L0R32

If you rebase this branch on top of that one, the tests pass.

@seancribbs

👍 then 😄

@Vagabond Vagabond merged commit 2a8706b into master Feb 26, 2013

1 check passed

Details default The Travis build passed
@seancribbs seancribbs deleted the adt-variable-arguments branch Apr 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment