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

Index variable definitions and usages #492

Closed

Conversation

scottming
Copy link
Collaborator

This PR adopts a combination of operators to extract variable definitions and usages. There are a total of 4 commits, which should address most cases. However, I have not yet handled the else block. I would like to gather some feedback in advance.

The "=" sign is the simplest and most commonly used pattern matching symbol. In most cases, we only need to consider the content on the left side of the "=" sign, which includes variable definitions.
@scottming scottming changed the title Variable definitions and usages Index variable definitions and usages Nov 17, 2023
Copy link
Collaborator

@zachallaun zachallaun left a comment

Choose a reason for hiding this comment

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

I still need to think this through some more, but I've been thinking about the relationship between analysis and indexing/extractors:

  • analysis is all about pre-analyzing scopes and tying definitions to a position. It should replace the notion of blocks in extractors.
  • extractors should possibly have two callbacks: 1) that takes an analysis and returns any definitions that should be indexed, 2) the existing extract callback that now focuses on finding usages within the ast
  • I started working on tracking imports during analysis last night and made some good progress on a small refactor that will make it easier to extract different features and associate them with the correct scope/definition (e.g. an argument in a fun definition, or a variable binding in a block)
  • this would change what extractors need to do, when they encounter a usage, they'd use the analysis to "look up" the associated definition so that they can get its ref for indexing

I think the important bit is that analysis should contain the canonical definition of "scope" -- the indexer shouldn't have to do any scope tracking, it should just walk the ast and collect any usages we care about, and be able to refer to the analysis for definitions. Basically, extractors ask analysis, "I found this usage of foo at this position, what does it refer to?"

end

defp arrow_start_position(node) do
start_node = %Zipper{node: node} |> Zipper.leftmost() |> Zipper.next() |> Zipper.node()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
start_node = %Zipper{node: node} |> Zipper.leftmost() |> Zipper.next() |> Zipper.node()
start_node = node |> Zipper.zip() |> Zipper.leftmost() |> Zipper.next() |> Zipper.node()

scottming and others added 4 commits November 18, 2023 21:44
…side of the "=" sign.

All variable usages follow this format: {variable_atom, meta, nil}. Therefore, we have used the process of elimination here. If a position is not a definition, it is considered a usage. To find the definition, we need to obtain the actual block at the current position.Our previous current_block function did not accurately extract the current block or scope. This commit implements the true extraction of the current block.
The extraction of function header variables can reuse the logic of extract_from_left, but the difference from "=" is that we also need to extract variables from the right-hand side.
…tor.

We should not only implement scope for anonymous functions, but also for the -> operator. This way, we can handle cases like case and cond as well.
@scottming scottming force-pushed the variable-definitions-and-usages branch from 94a5ee3 to b1f010f Compare November 18, 2023 13:44
@scottming
Copy link
Collaborator Author

@zachallaun

I think the important bit is that analysis should contain the canonical definition of "scope" -- the indexer shouldn't have to do any scope tracking, it should just walk the ast and collect any usages we care about, and be able to refer to the analysis for definitions. Basically, extractors ask analysis, "I found this usage of foo at this position, what does it refer to?"

I mostly agree with your point. It makes sense to separate the logic of defining scope and the extractor.

While implementing this PR, I noticed that our reducer does not have the concept of ancestors. So, I implemented a simple version of it. However, constructing it every time we search for definitions leads to poor performance.

If the analysis could provide answers to these questions you mentioned, our work would undoubtedly be much simpler.

@zachallaun
Copy link
Collaborator

While implementing this PR, I noticed that our reducer does not have the concept of ancestors. So, I implemented a simple version of it. However, constructing it every time we search for definitions leads to poor performance.

If the analysis could provide answers to these questions you mentioned, our work would undoubtedly be much simpler.

Yep! I certainly believe analysis could/should be answering these going forward. I'll look into what changes need to be made to analysis to support this.

@scottming scottming closed this Feb 16, 2024
@scottming scottming deleted the variable-definitions-and-usages branch April 6, 2024 01:36
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.

2 participants