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

ide_db: build symbol index from crate def map #10872

Merged
merged 24 commits into from
Nov 30, 2021

Conversation

jhgg
Copy link
Contributor

@jhgg jhgg commented Nov 27, 2021

fixes #4842, #10764

Is this looking correct? 👀

  • build the symbol index based upon the CrateDefMap for the given crate in crate_symbols
    • make it multi threaded again, and figure out how to cache each moduleid's symbol index in salsa.
    • NavigationTarget for names in macros is wrong, need to figure out how to compute a text range in the original file id?
    • cleanup some duped code
    • collect macros from ItemScope.declared_macros() into symbol index.
      • store declared macros in ItemScope so we can figure out where macros were defined for the index.
    • do something about SymbolIndex::for_files - ideally it should use the new module symbol index stuff.
      • delete source_file_to_file_symbols & co...
        • figure out what to do about library_symbols
        • maybe... speed up the new library_symbols - the new impl is probably much slower, and definitely much less parallel. deciding to do nothing here, we can optimize later if necerssary.
    • fix failing test: navigation_target::tests::test_nav_for_symbol - notably the crate def map doesn't seem to find declarations inside function.
      • now a bunch of other tests are failing around auto_import & qualify_path handlers. :(
        • need to assoc items in traits and impls

@jhgg
Copy link
Contributor Author

jhgg commented Nov 28, 2021

some questions:

  1. it looks like library_symbols is a bit bespoke, insofar as it operates directly on re-parsed source text. is there a specific reason for this? would changing it to use a def map be problematic?

    • it looks like library_symbols additionally does not seem to have a way to access RootDatabase? if not, what would be the advisable strategy to parallelize. (i figure i need a RootDatabase to do a snapshot to distribute to worker threads)
  2. the new def map based symbol index does not descend into function bodies to find symbols, this has caused a test to fail. any tips on how to resolve? 8850ea0

@jhgg jhgg marked this pull request as ready for review November 29, 2021 09:36
crates/ide/src/navigation_target.rs Outdated Show resolved Hide resolved
crates/ide_db/src/symbol_index.rs Outdated Show resolved Hide resolved
crates/ide_db/src/symbol_index.rs Outdated Show resolved Hide resolved
crates/ide_db/src/symbol_index.rs Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Nov 30, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 30, 2021

@bors bors bot merged commit 2d0db31 into rust-lang:master Nov 30, 2021
@jhgg jhgg deleted the ide/symbol-index-from-def-map branch November 30, 2021 19:10
bors bot added a commit that referenced this pull request Jan 12, 2022
11266: internal: Mostly restore `hir` API boundary r=jonas-schievink a=jonas-schievink

The boundary was broken in #10872 by reexporting a bunch of `hir_def`-internal types.

This PR moves symbol collection to `hir` and removes those reexports again.

bors r+

Co-authored-by: Jonas Schievink <jonas.schievink@ferrous-systems.com>
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.

Symbol index should use DefMap instead of parsing files directly
4 participants