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.

Sign up for GitHub

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

Jump to bottom

Track start and end column for symbols #304

Closed
wants to merge 8 commits into from
Closed

Track start and end column for symbols #304

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1ed773
Add start_column and end_column to symbols table
crbelaus Oct 25, 2023
0c494e5
Record start and end column when saving symbols
crbelaus Oct 25, 2023
75737b0
Use start and end column when querying symbols
crbelaus Oct 25, 2023
cc660fd
Remove unwanted 'dbg' call
crbelaus Oct 25, 2023
f5fbd49
Try to infer defmodule/defstruct columns from references
crbelaus Oct 27, 2023
d6085f4
ci: add 1.16.0-rc.0
mhanberg Oct 31, 2023
c51dcdc
Merge branch 'main' into track-symbol-columns
mhanberg Oct 31, 2023
8752e19
Merge branch 'main' into track-symbol-columns
mhanberg Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/next_ls.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,7 @@ defmodule NextLS do
FROM "symbols" sym
WHERE sym.file = ?
AND sym.line = ?
AND (sym.type in ('defstruct', 'defmodule') OR ? BETWEEN sym.start_column AND sym.end_column)
ORDER BY sym.id ASC
LIMIT 1
"""
Expand All @@ -960,7 +961,7 @@ defmodule NextLS do
LIMIT 1
"""

case DB.query(database, definition_query, [file, line]) do
case DB.query(database, definition_query, [file, line, col]) do
[[module, "defmodule", _]] ->
{:module, module}

Expand Down
28 changes: 16 additions & 12 deletions lib/next_ls/db.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,10 @@ defmodule NextLS.DB do
__query__(
{conn, s.logger},
~Q"""
INSERT INTO symbols (module, file, type, name, line, 'column', source)
VALUES (?, ?, ?, ?, ?, ?, ?);
INSERT INTO symbols (module, file, type, name, line, start_column, end_column, source)
VALUES (?, ?, ?, ?, ?, ?, ?, ?);
""",
[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 •
edited
Loading

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.

)

if struct do
Expand All @@ -105,32 +105,36 @@ defmodule NextLS.DB do
__query__(
{conn, s.logger},
~Q"""
INSERT INTO symbols (module, file, type, name, line, 'column', source)
VALUES (?, ?, ?, ?, ?, ?, ?);
INSERT INTO symbols (module, file, type, name, line, start_column, end_column, source)
VALUES (?, ?, ?, ?, ?, ?, ?, ?);
""",
[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.

)
end

for {name, {:v1, type, _meta, clauses}} <- defs, {meta, _, _, _} <- clauses do
# Elixir versions older than 1.16 did not provide column metadata
column_start = meta[:column] || 1
name = to_string(name)

__query__(
{conn, s.logger},
~Q"""
INSERT INTO symbols (module, file, type, name, line, 'column', source)
VALUES (?, ?, ?, ?, ?, ?, ?);
INSERT INTO symbols (module, file, type, name, line, start_column, end_column, source)
VALUES (?, ?, ?, ?, ?, ?, ?, ?);
""",
[mod, file, type, name, meta[:line], meta[:column] || 1, source]
[mod, file, type, name, meta[:line], column_start, column_start + String.length(name), source]
)
end

for {type, name, line, column} <- symbols do
__query__(
{conn, s.logger},
~Q"""
INSERT INTO symbols (module, file, type, name, line, 'column', source)
VALUES (?, ?, ?, ?, ?, ?, ?);
INSERT INTO symbols (module, file, type, name, line, start_column, end_column, source)
VALUES (?, ?, ?, ?, ?, ?, ?, ?);
""",
[mod, file, type, name, line, column, source]
[mod, file, type, name, line, column, column + String.length(name), source]
)
end

Expand Down
5 changes: 3 additions & 2 deletions lib/next_ls/db/schema.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ defmodule NextLS.DB.Schema do

alias NextLS.DB

@version 5
@version 6

def init(conn) do
# FIXME: this is odd tech debt. not a big deal but is confusing
Expand Down Expand Up @@ -73,7 +73,8 @@ defmodule NextLS.DB.Schema do
type text NOT NULL,
name text NOT NULL,
line integer NOT NULL,
column integer NOT NULL,
start_column integer NOT NULL,
end_column integer NOT NULL,
source text NOT NULL DEFAULT 'user',
inserted_at text NOT NULL DEFAULT CURRENT_TIMESTAMP
);
Expand Down
Loading