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

Track start and end column for symbols #304

Closed
wants to merge 8 commits into from

Conversation

crbelaus
Copy link
Contributor

This pull request builds on the improvements made on elixir-lang/elixir#13029 to add column info in Module.get_definition/2. The goal is to record the start_column and end_column of symbols just like we do for references.

This should allow us to know exactly what's under the cursor when finding references and hopefully fix #184.

For the moment I did two changes:

  1. I've updated the schema so the symbols table has start_column and end_column instead of the previous column. When storing symbols we populate both columns.
  2. When querying what's under the cursor with symbol_info/4 we now take into account the new columns.

There is one caveat: we don't have column information for defmodule and defstruct so in those cases the start and end column is still 1 and we don't consider it when querying for symbols.

This makes me wonder if it would be better to make start_column and end_column nullable and just leave it empty in such cases. I believe it would be more accurate as we wouldn't be storing an incorrect column that we have to ignore when querying.

""",
[mod, file, "defmodule", mod, module_line, 1, source]
[mod, file, "defmodule", mod, module_line, 1, 1, source]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if it would be better to just not store the start_column and end_column in this case. The current information is not accurate and it may be more confusing than not having it.

This would also make it easier to query for symbols as we would just return the results in which the column matches or there is no column. Currently we have to discard defmodule and defstruct manually when querying.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, for defstruct, I believe we can query the reference table to get the start/end column information. i think that since the module is now compiled, all of the references in it should already have been traced.

For defmodule, it seems like only calls to defmodule that are inside another module are traced (not sure if that is a bug or an implementation detail), but you can most likely use it for column info for sub moduled, modules, and then if that doesn't exist, just assume its normal and its at column 1 (or 0, i can't remember how we store it)

Copy link
Contributor Author

@crbelaus crbelaus Oct 27, 2023

Choose a reason for hiding this comment

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

I've just added a new commit that does this and it seems to work fine both for defstruct and nested defmodule declarations. For top level defmodule declarations we don't have this information and are storing both start_column and end_columns as 1.

I tested it by adding the following module and confirming that every defstruct and the nested defmodule has the right columns.

defmodule NextLS.ParentModule do
  @moduledoc false
  defstruct id: nil, name: nil

  defmodule NestedModule do
    @moduledoc false
    defstruct nested_id: nil, nested_name: ""
  end
end

I was thinking that we could do a little bit better as we know that the start column is 11 ("defmodule " has 10 characters) and we can add the module name length to get the end column. Not sure about this though as it seems too "clever" and fragile.

""",
[mod, file, "defstruct", "%#{Macro.to_string(mod)}{}", meta[:line], 1, source]
[mod, file, "defstruct", "%#{Macro.to_string(mod)}{}", meta[:line], 1, 1, source]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@crbelaus crbelaus marked this pull request as ready for review October 25, 2023 17:30
@mhanberg
Copy link
Collaborator

I am testing this on 1.16 and there are a lot of test failures

@mhanberg
Copy link
Collaborator

I merged main and added the new 1.16 rc to CI

@mhanberg
Copy link
Collaborator

Actually, the tests are failing on 1.15 as well, but for some reason its outputting exit code 0 so CI thinks they passed.

Same happening for me locally

@mhanberg
Copy link
Collaborator

Alright, something is actually messed up on main, not sure what, but the tests fail with some strange error i've never seen before on my Intel Linux PC as well as M1 MacBook, so something has to be up.

@mhanberg
Copy link
Collaborator

i fixed on main and pushed it up to your branch

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.

Can't find references of the contents of an inline function
2 participants