[erlang-patches] Print column numbers when compiling

Tue Aug 28 15:28:59 CEST 2012

Hello,

See responses inlined.

Lukas

On 24/08/12 12:22, Anthony Ramine wrote:
> Most OTP (at least erl_lint, erl_parse and syntax_tools) already knows that locations can be a line or a line and a column; furthermore it is said in erl_parse itself that one should always use get_line/1 and set_line/3 instead of depending on having an integer there.
It does indeed say so inside the code of erl_parse. But all of the 
visible documentation (here[1] and here[2]) say that it is an integer. 
So if you have only read the documentation (which I assume most people 
only have) you would assume that it is an integer. That documentation 
has to be updated so that LINE can be either an integer() or a 
tuple(integer(),integer()), and a reference to use 
erl_parse:set/get_attribute to manipulate it safely.

The compiler should also be updated to use these API's, this includes 
the convention that a line number of -1 is used to say that something is 
compiler generated. To make sure that all uses of the non-api is 
eliminated, it might be a good idea to declare erl_scan:line as -opaque 
and let dialyzer search through the otp code for errors.

[1]: http://www.erlang.org/doc/apps/erts/absform.html
[2]: http://www.erlang.org/doc/man/erl_scan.html#type-line
>
> If it is really that heavy of a change, why do we have erl_scan:set_attribute/3 and erl_scan:attributes_info/{1,2}? They obviously were coded to mitigate these issues.
We have indeed been working on getting this to work before, but the 
effort was left as you see it now because there were a number of issues 
which needed solving and more important things got in the way. The 
reason why I say it is a heavy change is that we want to feel that a 
good effort has been made to make sure that all error and warning column 
numbers point to the correct position.
>
> Can't this option be included as part of an experimental feature and bugs be fixed as they emerge? It's not like it is mandatory to use it, nor like there is not already experimental and dubious code in OTP (parameterized modules I'm looking at you).
Just because we previously have made bad decisions in including 
experimental features, does not mean we should do it again. We want to 
be sure we have done everything we can to eliminate errors before 
releasing this.
>
> Any parse transform that break because of this change will break because it does AST manipulation wrong and their users will just have to not use the "column" option for it to continue to work.
Strictly speaking parse transform is also an experimental/undocumented 
feature of Erlang/OTP. But unfortunately a lot of people seem to use 
them, so we have to take some backwards compatibility into 
consideration. The closest thing to a documentation is the example found 
here lib/stdlib/example/erl_id_trans.erl, and that does not deal with 
line numbers at all.

We really really have to be sure that the fun2ms and qlc parse 
transforms work.
>
> I'm not deterred at all from getting this patch into OTP and I would like to know exactly which tests should I update to make you guys happy.
Great to hear! I'll do my best to be quick and correct when responding 
to questions.
>
> Regards,
>
> --
> Anthony Ramine
>
> Le 22 août 2012 à 17:30, Lukas Larsson a écrit :
>
>> Hi,
>>
>> We have now looked at this patch and there are a couple of issues remaining which have to be solved.
>>
>> The patch changes the representation of the abstract format which has a couple of implications:
>>
>> * Any parse transform which uses the line number position might brake when this new option (this could include OTP parse transforms)
>> * Any tool which depend on having an integer as the line number position will break (syntax tools might be one such tool)
>>
>> In order to mitigate this backwards incompatibility, we suggest making the line/column number an opaque datatype which is only accessible via an api and update the documentation to reflect this.
>>
>> In addition all of the different warnings/errors in erl_lint and friends have to be checked with the new column number so that they point to the correct position.
>>
>> Also the erl_lint, epp and any other suite which deal with error messages/warnings have to be updated to run tests with and without this option.
>>
>> I'm sure that I've forgotten some things which have to be done. This is a major change and will take some time to get correct.
>>
>> I hope I have not deterred you from getting this patch into the Erlang/OTP. If you have any questions about anything I will do what I can to help you.
>>
>> Lukas
>>
>> On 10/06/12 13:24, nox wrote:
>>> I added a commit to correctly order locations in compile:messages_per_file/1
>>> which didn't take into account column numbers.
>>>
>>> git fetch https://github.com/nox/otp.git compile-column-numbers
>>>
>>>
>>> The following changes since commit 9bbda97f63ba4ee7cd58c266ee69af1059352189:
>>>
>>>   Merge branch 'raimo/tools/remove-fprof-tuple-funs/OTP-10091' into
>>> maint (2012-05-23 16:15:58 +0200)
>>>
>>> are available in the git repository at:
>>>
>>>
>>>   https://github.com/nox/otp.git compile-column-numbers
>>>
>>> for you to fetch changes up to d932ad582676f1593d917f222b1d8dec71fe88b3:
>>>
>>>   Fix messages ordering with column numbers (2012-06-10 13:01:50 +0200)
>>>
>>> ----------------------------------------------------------------
>>> Anthony Ramine (7):
>>>       Export type erl_scan:location/0
>>>       Allow setting of initial position in epp
>>>       Create a new "column" option in compile
>>>       Fix printing of errors with column numbers
>>>       Test column number reporting in error_SUITE
>>>       Fix type compile:err_info/0
>>>       Fix messages ordering with column numbers
>>>
>>> lib/compiler/doc/src/compile.xml  |    5 +++++
>>> lib/compiler/src/compile.erl      |   25 +++++++++++++++++----
>>> lib/compiler/test/error_SUITE.erl |   15 +++++++++++--
>>> lib/stdlib/doc/src/epp.xml        |    2 ++
>>> lib/stdlib/src/epp.erl            |   44 +++++++++++++++++++++++++++++--------
>>> lib/stdlib/src/erl_scan.erl       |    2 +-
>>> 6 files changed, 77 insertions(+), 16 deletions(-)
>>>
>>> --
>>> Anthony Ramine
>>> _______________________________________________
>>> erlang-patches mailing list
>>> erlang-patches@REDACTED
>>> http://erlang.org/mailman/listinfo/erlang-patches
>>>
> _______________________________________________
> erlang-patches mailing list
> erlang-patches@REDACTED
> http://erlang.org/mailman/listinfo/erlang-patches
>