mirror of
https://github.com/Myriad-Dreamin/tinymist.git
synced 2025-11-22 12:34:39 +00:00
docs: improve code comments in expr.rs and RefExpr struct (#2169)
Some checks are pending
tinymist::auto_tag / auto-tag (push) Waiting to run
tinymist::ci / Duplicate Actions Detection (push) Waiting to run
tinymist::ci / Check Clippy, Formatting, Completion, Documentation, and Tests (Linux) (push) Waiting to run
tinymist::ci / Check Minimum Rust version and Tests (Windows) (push) Waiting to run
tinymist::ci / prepare-build (push) Waiting to run
tinymist::ci / announce (push) Blocked by required conditions
tinymist::ci / build (push) Blocked by required conditions
tinymist::gh_pages / build-gh-pages (push) Waiting to run
Some checks are pending
tinymist::auto_tag / auto-tag (push) Waiting to run
tinymist::ci / Duplicate Actions Detection (push) Waiting to run
tinymist::ci / Check Clippy, Formatting, Completion, Documentation, and Tests (Linux) (push) Waiting to run
tinymist::ci / Check Minimum Rust version and Tests (Windows) (push) Waiting to run
tinymist::ci / prepare-build (push) Waiting to run
tinymist::ci / announce (push) Blocked by required conditions
tinymist::ci / build (push) Blocked by required conditions
tinymist::gh_pages / build-gh-pages (push) Waiting to run
## Overview This PR enhances documentation in the expression analysis system to clarify how goto_definition works and when the `RefExpr` fields are set versus `None`. This addresses feedback from @BlueQuantumx and @jo3-l who requested better documentation to understand the expression analysis flow. ## Problem The existing documentation was insufficient for developers trying to understand: 1. How the goto_definition feature works through the expression analysis system 2. When the `root`, `step`, and `term` fields in `RefExpr` are `Some` versus `None` 3. The overall flow of expression analysis in `expr_of` The brief one-line comments didn't provide enough context about the resolution chain concept or concrete examples of when fields are populated. ## Changes ### RefExpr Structure Documentation (`crates/tinymist-analysis/src/syntax/def.rs`) Added comprehensive documentation including: - Explanation of the resolution chain concept: `root` -> `step` -> `decl` - Three concrete examples showing different use cases: - Simple identifier reference (`let y = x`) - Module field access (`mod.field`) - Import with rename (`import: old as new`) - Detailed field documentation explaining when each is set: - `step`: Set for imports, field access, chained references, renamed imports - `root`: Set for module imports, field selection, propagated in chains - `term`: Set when type inference succeeds, `None` when type unknown or deferred ### Expression Analysis Documentation (`crates/tinymist-query/src/syntax/expr.rs`) Enhanced `expr_of` function documentation to explain: - Two-pass analysis architecture (init_stage for forward references, then full resolution) - How it builds the resolves map that powers goto_definition - Caching strategy based on source content and import hashes Added detailed documentation for helper functions: - `resolve_ident_`: Resolution process and reference chain building - `eval_ident`: Lexical scope lookup order and type availability - `extract_ref`: Reference chain propagation mechanism Added inline comments at all RefExpr creation sites explaining the context and field values for: - Module imports - Import/include paths - Named imports with rename - Module field selection ## Example **Before:** The RefExpr struct had minimal documentation ```rust /// The intermediate step in resolution (if any). pub step: Option<Expr>, ``` **After:** Clear explanation of when the field is set ```rust /// The intermediate expression in the resolution chain. /// /// Set in the following cases: /// - **Import/include**: The module expression being imported /// - **Field access**: The selected field's expression /// - **Chained references**: When an identifier resolves to another reference /// - **Renamed imports**: The original name before renaming /// /// `None` when the identifier is a direct definition (not a reference). pub step: Option<Expr>, ``` ## Testing - ✅ All core tests pass (expr module, tinymist-analysis package) - ✅ Clippy passes with no warnings - ✅ Build succeeds - ✅ Code formatted with `yarn fmt` Note: Package-related test failures (cetz, fletcher, tidy, touying, import_package) are expected in environments without network access per project guidelines. > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `packages.typst.org` > - Triggering command: `/home/REDACTED/work/tinymist/tinymist/target/debug/deps/tinymist_query-1cacdd437723df09 --skip=e2e` (dns block) > - Triggering command: `/home/REDACTED/work/tinymist/tinymist/target/debug/deps/tinymist_query-1cacdd437723df09 goto_definition::tests::test --exact` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/Myriad-Dreamin/tinymist/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Improve code comments in expr.rs</issue_title> > <issue_description>> I'd like to push this forward, but I need some tips/docs about how the goto_definition works. I've walked through `fn def_of_syntax_or_dyn`, `fn definition` ... and finally focused on `crates/tinymist-query/src/syntax/expr.rs -> fn expr_of`. Maybe there could have some concise code comments? > > _Originally posted by @BlueQuantumx in [#1960](https://github.com/Myriad-Dreamin/tinymist/issues/1960#issuecomment-3294723078)_ > > > I read those comments when I was originally writing the code, but I'm mainly interested in when `type` and `root` are set (or, conversely, when they are `None`.) The comments don't really go into that detail AFAICS? > > _Originally posted by @jo3-l in https://github.com/Myriad-Dreamin/tinymist/pull/2065#discussion_r2315906981_</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> Fixes Myriad-Dreamin/tinymist#2122 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/Myriad-Dreamin/tinymist/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Myriad-Dreamin <35292584+Myriad-Dreamin@users.noreply.github.com> Co-authored-by: Myriad-Dreamin <camiyoru@gmail.com>
This commit is contained in:
parent
baca369c29
commit
d80199d744
3 changed files with 171 additions and 9 deletions
|
|
@ -841,6 +841,7 @@ impl SharedContext {
|
|||
res
|
||||
}
|
||||
|
||||
/// Gets the definition from a source location.
|
||||
pub(crate) fn def_of_span(
|
||||
self: &Arc<Self>,
|
||||
source: &Source,
|
||||
|
|
@ -851,6 +852,7 @@ impl SharedContext {
|
|||
definition(self, source, doc, syntax)
|
||||
}
|
||||
|
||||
/// Gets the definition from a declaration.
|
||||
pub(crate) fn def_of_decl(&self, decl: &Interned<Decl>) -> Option<Definition> {
|
||||
match decl.as_ref() {
|
||||
Decl::Func(..) => Some(Definition::new(decl.clone(), None)),
|
||||
|
|
@ -859,6 +861,10 @@ impl SharedContext {
|
|||
}
|
||||
}
|
||||
|
||||
/// Gets the definition from static analysis.
|
||||
///
|
||||
/// Passing a `doc` (compiled result) can help resolve dynamic things, e.g.
|
||||
/// label definitions.
|
||||
pub(crate) fn def_of_syntax(
|
||||
self: &Arc<Self>,
|
||||
source: &Source,
|
||||
|
|
@ -868,6 +874,13 @@ impl SharedContext {
|
|||
definition(self, source, doc, syntax)
|
||||
}
|
||||
|
||||
/// Gets the definition from static analysis or dynamic analysis.
|
||||
///
|
||||
/// Note: while this has best quality in typst, it is expensive.
|
||||
/// Use it if you know it is only called `O(1)` times to serve a user LSP
|
||||
/// request, e.g. resolve a function definition for `completion`.
|
||||
/// Otherwise, use `def_of_syntax`, e.g. resolves all definitions for
|
||||
/// package docs.
|
||||
pub(crate) fn def_of_syntax_or_dyn(
|
||||
self: &Arc<Self>,
|
||||
source: &Source,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue