Serve pull diagnostics and unify push/pull/CLI diagnostic sets - #329
abilliontokens wants to merge 3 commits into
Conversation
Pull clients (oh-my-pi LSP diagnostics) always saw a clean bill: no diagnosticProvider was advertised, so textDocument/diagnostic answered method_not_found. Pull now serves the same set the push path publishes (syntax, call-arg, nullable, when, missing-import, unused-import, missing-package) through one shared coordinator, waits bounded (18s, inside omp's default request timeout) for a cold-start workspace scan before answering, and works for never-opened files. kmp-lsp diagnose also runs unused-import and missing-package now (previously LSP-only), with --only support for both.
…atterns crawl scoping missing-import flagged kotlin.* top-level functions (lazy, repeat, with) whenever no stdlib JAR/sources were indexed while a same-named workspace declaration made them look importable. They are now exempt as a language fact in resolvable_via_default_import, mirroring the hardcoded default-import type list. The Tier-1 promotion regression test now probes with runCatching (outside the name-level list) so it still guards the promote-before-read path. workspace.json gains sourceJarPatterns: substring filters on group.artifact limiting which Gradle sources JARs get parsed (absent = unscoped). The global cache holds every project on the machine; a full-cache warmup plateaued at ~3.4GB RSS.
nullable-dot diagnostic attributed receivers via file-wide name scan, so a same-named declaration in another function (tile: CoordGrid? parameter vs val tile = free.removeAt(...)) poisoned every use. Simple-identifier receivers now resolve through the lexically visible declaration; unresolvable visible locals suppress instead of guessing. call-arg diagnostic flagged calls kotlinc accepts through overridden declarations' defaults (verified by compiling: Player.spotanim drops PathingEntity.spotanim defaults, yet 3-arg calls compile). When the resolved signature rejects, same-name members on the receiver's or enclosing scope's hierarchy chain are tested for acceptance first.
|
Great find ! thanks a lot for PR, I'll review it as soon as possible |
Hessesian
left a comment
Hessesian
left a comment
There was a problem hiding this comment.
Thanks for this — the pull-diagnostics work is solid and the false positives you chased down are real. I checked the PR against AGENTS.md, docs/agent-reference.md and docs/architecture/unified-resolution-strategy.md. Inline comments have the details; the overall suggestion is below.
Please split this into separate PRs
The title covers pull diagnostics, but the PR carries five independent changes. They have very different risk levels, and bundling them means the good parts wait on the contentious ones.
- Pull diagnostics + shared coordinator (
diagnostics.rs, capability, handler,when_diagnostics_with_doc, workspace handler call sites). This is close to mergeable. Two things to fix: routecli/run.rsthrough the coordinator too (the description says the CLI funnels through it, but it still hand-assembles its own list), and reconsider the hardcoded 18 s wait. - Stdlib default-import function exemption. Fine in principle; derive the name set from the existing tables in
src/stdlib.rsinstead of a second hand-maintained list. sourceJarPatterns. A new config feature, deserves its own PR and discussion. Reuseparse_workspace_datarather than adding a parallel loader.- Nullable cross-function leak. Real bug, and the
ScopedLocalthree-way outcome is the right idea. But the implementation re-parses source lines with string matching, where the project rule is to work on the CST and the codebase already has a scope-correct CST walker for exactly this problem. See the inline comment for a much smaller route. - Call-arg override tolerance. Real false positive, but the fix reads
indexer.definitions/indexer.filesby bare name with no reachability filter, and half of it (the unqualified path) is unreachable in production by the PR's own test comment.
For 4 and 5, some context: resolution logic in this repo is being consolidated so that features stop growing their own private resolvers (docs/architecture/unified-resolution-strategy.md). #331 is actively reworking the same functions in resolver/infer.rs that this PR widens to pub(crate). I would rather land those two fixes on top of the shared CST/resolver primitives than add two new feature-local resolvers that then need migrating.
Conventions (apply across the PR)
- No abbreviated names (
AGENTS.md):recv,recv_raw,recv_col,recv_base,ty,eq,idx,occ_text,pos,diags,resp, and single-letter closure parameters (c,b,i). - Numbered section comments inside a function body (
// 1.,// 2.,// 3.inscoped_variable_type) signal that each phase wants to be a named helper. - Search before you write (agent-reference §12): several new helpers duplicate existing ones; each is flagged inline.
What is already good: tests live in companion *_tests.rs files, every fix has a test plus a negative guard, node kinds use the KIND_* constants, no unwrap() in production code, and the CHANGELOG and docs are updated. CI is green on all three platforms.
Happy to help with the CST-based version of 4 if useful.
| } | ||
|
|
||
| /// Core: type of `name` as declared visibly at `use_line`/`use_byte_col`. | ||
| fn scoped_variable_type( |
There was a problem hiding this comment.
This function and its helpers (is_value_declaration, is_value_or_param_declaration, assignment_eq_pos, bare_call_callee, find_ident_col) decide which declaration is in scope by string-matching source lines. local_scope_occurrences has already done a CST walk at this point; its result is then reduced to line numbers and the declaration is re-guessed from text.
There is an existing scope-correct CST walker for this exact problem: resolve_declared_type_from_cst(point, var_name, source) in src/resolver/infer.rs. Its doc comment describes this bug (several functions with same-named parameters, a whole-file scan cannot prefer the one in scope). The nullable diagnostic simply does not call it yet.
It cannot be used as-is, for two reasons:
- It returns
Option<String>, so "found an unannotated declaration in scope" and "found nothing" both come back asNone. YourScopedLocalenum is the right shape for that outcome and should move onto the walker. - It drops the
?from a nullable type (type_from_nullable), which this diagnostic needs.
Suggested route: extend that walker to return a three-way outcome (carrying the declaration node when there is no annotation, and preserving nullability), run infer_expr_type on the initializer node for the unannotated case, and call it from resolve_receiver before infer_receiver_type. That keeps the fix and removes most of the ~340 lines added here.
| } | ||
| // `name:` form, whole word, not a `::` callable reference. | ||
| let pattern = format!("{name}:"); | ||
| let mut search_from = 0; |
There was a problem hiding this comment.
Possible panic: search_from = pos + 1 followed by line[search_from..] slices in the middle of a UTF-8 character when name starts with a non-ASCII character and the first match is rejected. Example: a local named über used as a bound reference, über::run — after_ok is false, the loop advances one byte into ü, and the slice panics. find_ident_col below already uses line.get(search_from..)?; the same is needed here (or advance by the matched character's length).
| /// Type of an unannotated `val`/`var` from the initializer tables, filtered | ||
| /// to the visible declaration's own line — never another same-named | ||
| /// declaration's entry. | ||
| fn scoped_initializer_type( |
There was a problem hiding this comment.
Two things here:
- The receiver position is recovered by searching the declaration line for
=and then for the identifier text (assignment_eq_pos,find_ident_col), andbare_call_calleeparses the right-hand side by counting parentheses. With the declaration node in hand, the initializer is a CST child andinfer_expr_type(src/indexer/infer/expr_type.rs) already infers its type, including method and field hops. - The
method_matchandfield_matchblocks are near copies of each other (sameeq/recv_col/scoped_variable_typematch). If this stays, the shared part wants to be one helper.
|
|
||
| /// Reduce a type string to a bare class name for hierarchy/member lookup: | ||
| /// drop generics, outer qualifiers, and a trailing `?`. | ||
| fn bare_class_name(type_name: &str) -> String { |
There was a problem hiding this comment.
Duplicates existing StrExt helpers: dotted_ident_prefix().last_segment() plus strip_nullable() — the same chain resolve_method_return_type_substituted uses. The same inline chain is also repeated in scoped_initializer_type in the nullable file.
| /// Lexically enclosing scope types for an unqualified call, nearest first: | ||
| /// extension-receiver types of enclosing `fun Receiver.name` declarations | ||
| /// and names of enclosing classes/objects (implicit `this` scopes). | ||
| fn enclosing_scope_types(call_node: &tree_sitter::Node, bytes: &[u8]) -> Vec<String> { |
There was a problem hiding this comment.
Three concerns:
- The comment on
tolerance_accepts_bare_call_in_extension_scopesays the end-to-end version of this shape "passes vacuously — unqualified resolution already declines to diagnose". If so, this unqualified path cannot change any production result, and the function plus its direct-call test should be dropped until there is a case that needs it. node.parent()in a loop: tree-sitter'sparent()is O(depth), not a pointer dereference (agent-reference §18a). This cost a 217 s rename once.- Existing helpers cover both lookups:
enclosing_class_name(src/indexer/infer/chain.rs) andenclosing_extension_receiver_at(src/parser.rs).
Small bug as well: the dedupe checks types.contains(&base) but pushes bare_class_name(&base), so the check compares against the un-normalised form.
| } | ||
| } | ||
| } else { | ||
| // No index needed: `unused-import` is a pure CST walk and |
There was a problem hiding this comment.
The PR description says the CLI funnels through semantic_diagnostics / full_diagnostics, but run_diagnose still calls each feature itself, and the unused-import / missing-package block now exists twice (once per branch). DIAGNOSTIC_NAMES is kept in sync with the coordinator only by its doc comment.
That is the drift this PR sets out to remove (agent-reference §17: "every caller must remember to do X"). Suggestion: have the coordinator own the list of checks with their names and whether each needs the index, so --only filtering, the LSP paths and DIAGNOSTIC_NAMES all derive from one place.
| /// calls it unqualified without an import will no longer be told about it. | ||
| /// Names mirror the project's own curated stdlib inventory (`TOP_LEVEL_FUNS` | ||
| /// / `SCOPE_FUNS` in `src/stdlib.rs`), not a guess at the whole stdlib. | ||
| fn is_default_import_function(name: &str) -> bool { |
There was a problem hiding this comment.
The doc comment says these names mirror TOP_LEVEL_FUNS / SCOPE_FUNS in src/stdlib.rs. A hand-kept mirror will drift; please derive the check from those tables (a small stdlib::is_default_import_function(name) over the two slices) so there is one inventory.
| /// is not present — callers index every discovered sources JAR (unscoped). | ||
| /// Returns `Some(patterns)` when the key is present, including an explicitly | ||
| /// empty list (which parses no sources JARs at all). | ||
| pub(crate) fn load_source_jar_patterns(workspace_root: &Path) -> Option<Vec<String>> { |
There was a problem hiding this comment.
parse_workspace_data just below already does the exists / read / parse / log-warning sequence and returns the full WorkspaceData. This can be parse_workspace_data(workspace_root)?.source_jar_patterns.
Separately: index_sources_jars now reads workspace.json from disk on every call. The other workspace.json settings are resolved once at the workspace-config level and passed in; this one should follow the same route.
| &self, | ||
| params: DocumentDiagnosticParams, | ||
| ) -> Result<DocumentDiagnosticReportResult> { | ||
| const SCAN_WAIT_DEADLINE: std::time::Duration = std::time::Duration::from_secs(18); |
There was a problem hiding this comment.
The 18 s figure is tuned to one client's default request timeout and hardcoded in the server. Other pull clients will have different timeouts, and an editor that pulls on every change would block for up to 18 s per request during a scan. Could this be a much shorter default with an initialization option to raise it? A notification-based wait on scan completion would also be preferable to a 50 ms poll loop, but that can be a follow-up.
| /// supertype-walk fallback itself went missing from this file before it was | ||
| /// added back (see `find_method_return_type_via_supertypes`'s callers). | ||
| fn resolve_method_return_type_substituted( | ||
| pub(crate) fn resolve_method_return_type_substituted( |
There was a problem hiding this comment.
This and find_field_type_in_class_impl are internal steps of the string-engine inference, widened to pub(crate) so a feature can drive them directly. #331 is changing how these hops carry a type's origin, so a new external caller here will need migrating. Going through infer_expr_type / the Resolver trait instead would avoid exposing them.
2 participants
Pull-based clients (oh-my-pi's "LSP diagnostics") always saw a clean bill: the server advertised no
diagnosticProvider, sotextDocument/diagnosticansweredmethod_not_found. Worse, each diagnostics surface hand-assembled its own check list, and they had drifted — the CLI never ranunused-importormissing-package.What changed
textDocument/diagnostic(pull) now served:diagnosticProvidercapability + handler returning the same set the push path publishes, working for never-opened files vialive_doc_or_parse/disk fallback.src/features/diagnostics.rs): didOpen, republish, debounced-didChange, pull, and CLI all funnel throughsemantic_diagnostics/full_diagnostics.whensplit into a doc-based core (when_diagnostics_with_doc) so pull doesn't depend on a stored live tree.diagnoserunsunused-import+missing-packagewith--onlysupport; both skip the index build.docs/features.md),CHANGELOG.md(Unreleased), CLI help text updated.Verification
cargo build,cargo fmt --check,cargo clippy --all-targets --all-features -- -D warningsclean.smoke_pull_diagnostics_matches_push.kmp-jar-indexersidecar artifact CI fetches (absent in a bare checkout — documented inci.yml).connect: expected 2 argument(s), found 1andUnused import(4.3s silo-rooted); the realWildernessSkull.ktreturns a genuinely emptyfullreport.Notes
whenexhaustiveness, and import hygiene — still no full type checking (documented); deeper errors need Gradle/CI.~/.omp/lsp/kmp-lsp/kmp-lsp.exe, debug build) with this branch's binary to verify; that step is outside this PR.