lexical-lsp / lexical

Lexical is a next-generation elixir language server
782 stars 77 forks source link

Added variable extractor and ability to customize which extractors run #621

Closed scohen closed 4 months ago

scohen commented 4 months ago

Added an extractor that pulls out variables and references from AST. It's likely incomplete, but we can add missed cases as we go.

I also added the ability to customize which extractors run during an index. The idea here is to have a separate indexing run after a file is compiled, so we can pull out variables from it and power go to definition for variables without polluting the index with things that we won't use or need.

scohen commented 4 months ago

Additionally, how would you differentiate between multiple variables with the same name appearing in a single scope?

For the purposes of extraction, there's no handling to speak of, we gather every reference in the code, and store it. Filtering happens during lookup.

Once things are extracted, if you want to find a definition, you get all the definitions in the current scope, sort them by line and column, and find the nearest one that matches the variable name you're looking for. If that doesn't exist, you get the parent scope and try that again.

Finding references is much the same as definitions, but you carry an accumulator with you, and while the definition isn't found, add any references to the collection. Once a definition is found, you add only those references that occur after the definition.

scohen commented 4 months ago

I like the simplicity of this PR, but I'm curious, besides the unimplemented cross-scope variable extraction

What do you mean?

what other scenarios have we deliberately not implemented?

I've tried to be complete here, if something isn't implemented, please tell me.

scottming commented 4 months ago

What do you mean?

Once things are extracted, if you want to find a definition, you get all the definitions in the current scope, sort them by line and column, and find the nearest one that matches the variable name you're looking for

I think your answer above already addresses this question. I previously thought there was a need to differentiate the variable definitions for each scope.

scottming commented 4 months ago

I've tried to be complete here, if something isn't implemented, please tell me.

This PR doesn't seem to handle the variable definitions in these blocks, for example: [:else, :rescue, :catch, :after]

    test "When it appears in the else block of a with statement." do
      {:ok, [func, var], doc} =
        ~q[
        func = fn _ -> :not_ok end

        with :ok <- func.(1) do
          :ok
        else
          var -> var
        end
        ]
        |> index_definitions()

      assert_definition(func, :func)
      assert decorate(doc, func.range) =~ "«func» = fn _ ->"

      # NOTE: current PR doesn't handle the `var` in the `else` block
      assert_definition(var, :var)
      assert decorate(doc, var.range) =~ "  «var» ->"
    end
scohen commented 4 months ago

This PR doesn't seem to handle the variable definitions in these blocks, for example: [:else, :rescue, :catch, :after]

Correct me if i'm wrong, but I don't think you can define a variable in an after block.

The problem isn't really the blocks either, it's that the stab (->) operator wasn't handled properly. It was special-cased for the fn case. Removing the special case allows for more generic handling of defining variables in stabs.

scohen commented 4 months ago

@scottming I believe i've addressed the latest issues.