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 erl_anno:set_end_location/2 #8966

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

jonatanklosko
Copy link
Contributor

See the discussion in #8945 (comment).

Currently erl_anno:end_location(Anno) is inferred from location and text (if present). However, this means that in order to store a location range, we always need to store the whole text. This allows setting end_location of Anno directly.

With this change erl_anno:end_location(Anno) will first look for end_location set directly and then fallback to the inference if applicable.

Copy link
Contributor

github-actions bot commented Oct 18, 2024

CT Test Results

    2 files     96 suites   1h 7m 1s ⏱️
2 161 tests 2 113 ✅ 48 💤 0 ❌
2 520 runs  2 470 ✅ 50 💤 0 ❌

Results for commit fa58d2c.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

false ->
undefined;
{location, Location} ->
{end_location, erl_anno:end_location(erl_anno:set_text(Text, erl_anno:new(Location)))}
Copy link
Contributor Author

@jonatanklosko jonatanklosko Oct 18, 2024

Choose a reason for hiding this comment

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

The end_location is a bit tricky in the current tests setup, because it's both primary and secondary part of anno. In order to accommodate this, here we need to infer the value here (similarly to the clause for line above). Duplicating the whole text scan logic from the actual implementation sounds weird, so I call the inference directly.

I am happy to revise the tests, if another approach is preferable :)

Copy link
Contributor

@gomoripeti gomoripeti left a comment

Choose a reason for hiding this comment

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

great addition

lib/stdlib/src/erl_anno.erl Show resolved Hide resolved
@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Oct 21, 2024
@garazdawi
Copy link
Contributor

FYI #3026 also suggests adding this and has some other additions. I'll leave the decided on what to do to @bjorng and @jhogberg.

@bjorng
Copy link
Contributor

bjorng commented Oct 21, 2024

It is not clear to me from the discussion in #8945 who would actually call erl_anno:set_end_location/2? The scanner? A tool such as an editor?

So what I'm asking whether this new function would be useful without also updating the scanner and/or parser.

@jhogberg and I looked at #3026 and thought it would be useful to implement something like that to be able to provide better compiler messages and warnings, but we might want to implement in a slightly different way (with less heuristics). However, it is unlikely that will happen before Erlang/OTP 29.

@josevalim
Copy link
Contributor

This would not be used by the scanner but it could be used by the parser. The issue today is that we only keep token information from the scanner, but if we want to know the whole range of a function clause, we can't store that today. By adding this, we would allow the parser to annotate the end location (based on the . or ; or end tokens) when building the clause abstract format. Elixir would also use this to store similar information in the Docs chunk (that today uses erl_anno).

@bjorng bjorng added the testing currently being tested, tag is used by OTP internal CI label Nov 8, 2024
@bjorng
Copy link
Contributor

bjorng commented Nov 12, 2024

This looks good to me. Please squash the commits and force-push.

@jonatanklosko
Copy link
Contributor Author

Done!

@bjorng bjorng merged commit 119b3ff into erlang:master Nov 12, 2024
18 checks passed
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 testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants