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

Handle annotations and locations better #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Handle annotations and locations better #3

wants to merge 2 commits into from

Conversation

uabboli
Copy link

@uabboli uabboli commented Jan 13, 2021

See comment in first commit.

This PR is one of several affecting repositories on Github. It
aims at fixing bad use of annotations (see erl_anno(3)). The
following remarks are common to all PR:s.

Typically the second element of abstract code tuples is assumed
to be an integer, which is no longer always true. For instance,
the parse transform implementing QLC tables (see qlc(3)) returns
code where some annotations are marked as `generated'. Such an
annotation is a list, not an integer (it used to be a negative
integer).

As of Erlang/OTP 24.0, the location can be a tuple {Line, Column},
which is another reason to handle annotations properly.
Just to clearify that Pos is not necessarily an integer line number.
@jkrukoff
Copy link
Owner

jkrukoff commented Jan 19, 2021

I'm not sure if I'm looking at a human or computer generated PR here.

I can understand wanting to rename the internal variable from Line to Pos, especially given that it's meaning is changing. I believe the only thing I'm doing with the value from erl_syntax:get_pos is passing it into merl (and set_pos), however. A glance at the docs show merl already expecting an annotation value. This makes me suspicious that the transformation added here to extract a line number from the annotation is actually losing information, when instead the annotation should just be passed along as is.

As it's been quite a while since I last looked at the erl_syntax/merl documentation it's quite possible I'm missing something. Can you explain why this PR shouldn't just be a global rename from Line to Pos?

@uabboli
Copy link
Author

uabboli commented Jan 19, 2021

I'm not sure if I'm looking at a human or computer generated PR here.

Could you explain why you think so? It's an honest question; I'm
really curious.

I believe the only thing I'm doing with the value from
erl_syntax:get_pos is passing it into merl, however. A glance at the
docs show merl already expecting an annotation value.

According to merl(3), the type of the first argument of qquote() is
erl_anno:location(). The docs of erl_syntax:get_pos() says it returns
an integer, but the description is outdated (it'll be updated in
Erlang/OTP 24.0). It has always returned the second element of
abstract code tuples, the type of which is erl_anno:anno() since a
couple of releases.

As I wrote in the commit message, locations will be {Line, Column} by
default in Erlang/OTP 24.0. I don't think it affects the 'partial'
parse transform very much; the only thing I saw that is wrong is that
the return value of get_pos() is used in the error information.

Since the representation of erl_anno:anno() and erl_anno:location() is
the same almost always, the mix-up of the two opaque data types is
rather benign, but it should be corrected, I think.

To sum it up: you don't have to merge this PR; the code will not stop
working. But if someone calls, for example, the qlc parse transform
before 'partial', there is a very slight risk a list will be placed in
the error information instead of a location.

And I see now that I could (should) have used erl_anno:get_location()
in get_line/1 (which then of course should be named "get_location").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants