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

Analysis #457

Merged
merged 23 commits into from
Nov 17, 2023
Merged

Analysis #457

merged 23 commits into from
Nov 17, 2023

Conversation

zachallaun
Copy link
Collaborator

@zachallaun zachallaun commented Oct 31, 2023

Fixes #426

This PR introduces the concept of analysis, which is encapsulated by the %Lexical.Ast.Analysis{} struct. The idea is that, in the happy case, we can analyze the AST up front to capture important information and then re-use it for subsequent operations.

The most important new APIs/changes introduced:

  • Lexical.Ast.analyze(document) - returns %Analysis{} which may or may not be valid (analysis.valid?). Used for whole-document analysis, e.g. indexing.
  • Lexical.Ast.reanalyze_to(analysis, position) - returns %Analysis{} using the fragment of the original document up to the given position. Useful for things that may not require the whole document, e.g. resolving aliases. If the given analysis is already valid, this is a no-op.
  • Lexical.Ast.Aliases has been removed in favor of analysis.
  • Many Lexical.Ast functions have been refactored to also (or only) accept an analysis in place of a document.
  • Added concept of derived data to Lexical.Document.Store. This allows for a derived analysis to be saved alongside the document and fetched using {:ok, %Document{}, %Analysis{}} = Document.Store.fetch(uri, :analysis).
  • Refactored a number of existing LS features to use analysis, including entity resolution, completions, references.

Running the following extremely simple "benchmark" in iex -S mix:

# on main
iex> time fn -> Lexical.RemoteControl.Search.Indexer.create_index(project()); nil end
Time: 9.7 seconds

# on this branch
iex> time fn -> Lexical.RemoteControl.Search.Indexer.create_index(project()); nil end
Time: 1.8 seconds

apps/common/lib/lexical/ast/analysis/analyzer.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast/analysis.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast/analysis.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast/analysis.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast/analysis/analyzer.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast/analysis/analyzer.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast/analysis/analyzer.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast/analysis/analyzer.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast/analysis/analyzer.ex Outdated Show resolved Hide resolved
@zachallaun zachallaun force-pushed the za-analysis branch 3 times, most recently from 92cc5b8 to 8336ee4 Compare November 1, 2023 15:37
@zachallaun
Copy link
Collaborator Author

zachallaun commented Nov 2, 2023

Reminder to self: the pattern for detecting alias Foo, as: ... is incorrect, as other options can be passed as well, like warn: false.

edit: done

@zachallaun zachallaun force-pushed the za-analysis branch 3 times, most recently from 9822223 to d58a134 Compare November 3, 2023 16:41
@zachallaun zachallaun force-pushed the za-analysis branch 2 times, most recently from 0af5ef2 to b828358 Compare November 6, 2023 14:36
@zachallaun zachallaun marked this pull request as ready for review November 6, 2023 14:37
@zachallaun
Copy link
Collaborator Author

@scohen @scottming I'm going to mark this as ready for review, as I think it's very close to a merge-able state. I'll update the OP with some additional information.

Copy link
Collaborator

@scohen scohen left a comment

Choose a reason for hiding this comment

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

This change is excellent.

I have a couple of small comments, but I think we should move forward on this.

Makefile Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast.ex Outdated Show resolved Hide resolved
apps/common/test/lexical/ast/analysis_test.exs Outdated Show resolved Hide resolved
projects/lexical_shared/lib/lexical/document/store.ex Outdated Show resolved Hide resolved
This functionality is tested thoroughly at a higher level in alias resolution tests.
@zachallaun zachallaun merged commit face3c6 into main Nov 17, 2023
7 checks passed
zachallaun pushed a commit that referenced this pull request Nov 17, 2023
Find references weren't correctly updated to use analysis in #457.
@zachallaun zachallaun deleted the za-analysis branch April 21, 2024 16:52
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.

Indexing: Improve the performance of Aliases
2 participants