feat(lsp): add cross-file symbol navigation

[owenrumney]

Summary

• index variable definitions and uses across Make syntax for definition and reference navigation
• resolve included Makefiles from open buffers and refresh dependent diagnostics when they change
• add parser, resolver, diagnostics, and handler coverage for cross-file navigation

Closes #41

Testing

• go test ./...

[Copilot]

🟡 Changes recommended

Unresolved watcher, parser, and symbol-navigation issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds cross-file Make symbol navigation, include resolution from open buffers, and owner-aware diagnostics.

Changes:

• Indexes variable references and resolves definitions across included Makefiles.
• Tracks nested includes from buffers or disk.
• Adds diagnostics refresh, watcher handling, and test coverage.

File summaries

File	Summary	Review notes
vscode-make-ls/src/extension.ts	Delegates file watching to the language server.	—
internal/resolver/resolver.go	Resolves includes recursively with buffer-aware source tracking.	—
internal/resolver/resolver_test.go	Tests URI handling, safety, and source providers.	—
internal/parser/parser.go	Extracts positioned variable references.	Moderate (3 votes, line 477): parseDefine does not index references in define bodies.
internal/parser/parser_test.go	Covers reference indexing and ranges.	—
internal/model/model.go	Adds ownership, source, and reference metadata.	—
internal/handler/handler.go	Implements cross-file navigation, diagnostics, and watchers.	Critical (2 votes, line 84): non-dynamic clients lack watcher fallback. Moderate (1 vote, line 1015): macro body ranges are treated as declaration ranges. Moderate (3 votes, line 991): nested variable references can be parsed incorrectly. Moderate (1 vote, line 434): arbitrary include filenames are not watched.
internal/handler/handler_test.go	Tests navigation, diagnostics, and dependent refresh behavior.	—
internal/analysis/diagnostics.go	Routes diagnostics by owning URI.	—
internal/analysis/diagnostics_test.go	Tests reference exclusions and diagnostic handling.	—

Review details

Suppressed comments (2)

internal/handler/handler.go:1017

• Define.Range spans the whole block from define through endef, not just the macro name. This new branch therefore treats any cursor inside a macro body as being on the macro declaration; for example, References on $(CC) inside define greet ... endef returns references for greet instead of CC. Restrict this check to a name/header range so body references can be resolved.

	for _, d := range mf.Defines {
		if d.URI == uri && inRange(pos, d.Range) {
			return varRefs(d.Name), nil

internal/handler/handler.go:439

• The resolver accepts any included path, but this registration only watches *.mk, *.mak, Makefile, makefile, and GNUmakefile. Valid includes such as include config or include generated will never produce didChangeWatchedFiles, so open parent models and diagnostics remain stale when those files change outside the editor. Register exact included/unresolved paths or otherwise cover arbitrary include filenames.

	opts, err := json.Marshal(lsp.DidChangeWatchedFilesRegistrationOptions{
		Watchers: []lsp.FileSystemWatcher{
			{GlobPattern: "**/*.{mk,mak}"},
			{GlobPattern: "**/[Mm]akefile"},
			{GlobPattern: "**/GNUmakefile"},
		},

• Files reviewed: 10/10 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved include-resolution, parser-range, navigation, and file-refresh issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

internal/handler/handler.go:222

• Saving an included document reparses only its own model; it never calls reresolveDependents(uri). An open root that already merged the old content therefore keeps stale definitions, references, and diagnostics until another change or watcher event. Reuse the dependent re-resolve/publish path used by DidChange here.

	if text, ok := h.docs[uri]; ok {
		h.setParsed(uri, text)
	}

internal/parser/parser.go:869

• This nested-reference handling drops valid substitution references. varRefAt intentionally rejects function expressions and one-character automatic variables, so substRefAt returns false for valid forms such as $(SRC:%.c=$@/%.o) or $(SRC:%.c=$(dir $(OUTDIR))/%.o); the base SRC is then absent from Makefile.VarRefs, breaking definition/reference navigation. Skip balanced nested $(...)/${...} expressions without aborting on $@-style references.

		case '$':
			// $(SRC:%.c=$(OUTDIR)/%.o): step over the nested reference or the
			// outer name is lost.
			_, nestedEnd, nestedOK := varRefAt(s, j)
			if !nestedOK {
				return "", 0, false
			}

internal/parser/parser.go:543

• Conditional headers are parsed after parseLine joins continuations, but this still computes references against only p.lines[startLine] and linePos(startLine). For example, in ifeq \\\n ($(FOO),x), FOO is indexed on the first physical line instead of line 1, so definition/reference locations are wrong. Pass the joined header and its logicalPos through conditional parsing.

		VarRefs: p.parseConditionalVarRefs(condType, args, p.lines[startLine], linePos(startLine)),

vscode-make-ls/src/extension.ts:21

• Removing this watcher leaves LSP clients that do not advertise dynamic workspace/didChangeWatchedFiles registration with no file-event path: supportsWatchedFiles then disables registerFileWatchers, and the server advertises no static fallback. Those clients will not refresh an open root when an on-disk included Makefile changes. Keep a client-side watcher fallback or provide an equivalent static/polling path.

    documentSelector: [{ scheme: "file", language: "makefile" }],

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Lite