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

Add end_location annotations for parse trees #3026

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

Conversation

richcarl
Copy link
Contributor

@richcarl richcarl commented Feb 2, 2021

Based on #3020.

This allows the quoted source code in error messages to underline the range of the error, and can also be used by tools that display source code, as in an IDE.

Example:

foo.erl:18:16: illegal guard expression
%   18| foo(A, B) when is_lit(A), is_list(B) ->
%     |                ^^^^^^

instead of just

foo.erl:18:16: illegal guard expression
%   18| foo(A, B) when is_lit(A), is_list(B) ->
%     |                ^

This has only been implemented on the parser level. Adding further support for the scanner to annotate all tokens with end position right away would save the time now spent calculating end positions of tokens by taking the length of the text.

Can be suppressed with the 'brief' compiler option.
Moves message formatting code to a separate module.
This allows the quoted source code in error messages to underline the
range of the error, and can also be used by tools that display source code.
@uabboli uabboli self-assigned this Feb 3, 2021
@uabboli uabboli added the team:VM Assigned to OTP team VM label Feb 3, 2021
@@ -24,7 +24,7 @@
-export([column/1, end_location/1, file/1, generated/1,
line/1, location/1, record/1, text/1]).
-export([set_file/2, set_generated/2, set_line/2, set_location/2,
set_record/2, set_text/2]).
set_end_location/2, set_record/2, set_text/2]).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear OTP team,

Allowing end_location key as a first class citizen in erl_anno:anno() and the addition of erl_anno:set_end_location/2 would be beneficial in itself.
I started to need this since f30514e#diff-9f3b412e8fbd809c6473fcd5bee30f63931c7349010640d76316cfcde504e733
where erl_syntax:set_pos started to require a proper erl_anno:anno() instead of any term.
Even if the parser or OTP tools don't use it, other tools could store end_location annotation (without text).
This is not a major issue, but only to make dialyzer and erl_anno:is_anno/1 happy.

Would the OTP team be open to include only the erl_anno changes of this PR in OTP 24 or is it too late?
(minor benefit of including the set_pos and this change in the same release would be that there is a way before OTP 24 and from OTP24 to store start and end location in erl_syntax:set_pos, without a gap)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not certain that end_location is the best way to represent ranges in the AST, so we do not want to make a commitment to that design before we know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, thanks for the response
(out of curiosity what alternatives or drawbacks of end_location came up? I can think of text for all abstract format, not just some leafs)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall what the exact reasons were, but the alternative approach that we have been talking about is including the text that a node represents in anno, or something like that. We are not actively working on this, so my memory is a bit rusty.

@uabboli uabboli removed their assignment Aug 25, 2021
@potatosalad potatosalad deleted the source-code-ranges branch May 31, 2022 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants