Improve Makefile navigation for kernel-style conditionals and includes#21
Conversation
owenrumney
force-pushed
the
fix/tidy-up-more-references
branch
from
aedb436 to
b19b71a
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves Makefile navigation by enhancing parsing and resolution of kernel-style constructs so the language server can provide more accurate go-to-definition/references across directives, conditionals (including else-if), and computed include paths.
Changes:
- Resolve computed include paths (e.g.
$(srctree)/$(addprefix ...)) and carry resolved include locations into go-to-definition. - Parse else-if conditional chains and capture variable references in conditional headers and directives for navigation.
- Bump VS Code extension version and adjust extension install to use
--force.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vscode-make-ls/package.json | Bumps extension version for the new navigation features. |
| vscode-make-ls/package-lock.json | Keeps lockfile version in sync with the extension version bump. |
| Makefile | Forces VSIX reinstall during extension-install to ease iteration. |
| internal/resolver/resolver.go | Expands and resolves computed include paths and merges included Makefile models. |
| internal/resolver/resolver_test.go | Adds tests covering computed and addprefix include path resolution. |
| internal/resolver/eval.go | Introduces a small evaluator to expand a subset of Make functions for include resolution. |
| internal/parser/parser.go | Adds else-if parsing, directive/conditional var refs, and include-arg splitting that respects $() nesting. |
| internal/parser/parser_test.go | Adds tests for else-if parsing, computed includes, and conditional var-ref ranges. |
| internal/model/model.go | Extends Include with ResolvedPath and Conditional with VarRefs for navigation. |
| internal/handler/handler.go | Improves definition/references for directives/conditionals and uses resolved include paths for navigation. |
| internal/handler/handler_test.go | Adds integration-style tests for kernel-like snippets and computed include go-to-definition. |
Files not reviewed (1)
- vscode-make-ls/package-lock.json: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inc := &model.Include{ | ||
| Path: strings.TrimSpace(m[2]), | ||
| Path: paths[0], | ||
| Range: lineRange(lineNum, 0, len(fullLine)), | ||
| Optional: optional, | ||
| } |
There was a problem hiding this comment.
Fixed in follow-up. Conditional include parsing now preserves the full include expression instead of truncating to the first token, and computed/multi-include resolution is covered by tests.
| expanded := ctx.expandText(inc.Path) | ||
| if expanded == "" { | ||
| expanded = inc.Path | ||
| } | ||
| fields := strings.Fields(expanded) | ||
| if len(fields) == 0 { | ||
| fields = []string{expanded} | ||
| } |
There was a problem hiding this comment.
Fixed in follow-up. resolveIncludePaths now returns no paths for whitespace-only expansions, so it no longer falls back to a bogus path under baseDir.
| func extractDelimited(s string, start int) (int, string, bool) { | ||
| open := s[start] | ||
| closing := byte(')') | ||
| if open == '{' { | ||
| closing = '}' | ||
| } | ||
| depth := 1 | ||
| for i := start + 1; i < len(s); i++ { | ||
| if s[i] == '$' && i+1 < len(s) && (s[i+1] == '(' || s[i+1] == '{') { | ||
| depth++ | ||
| i++ | ||
| continue | ||
| } | ||
| if s[i] == closing { | ||
| depth-- | ||
| if depth == 0 { | ||
| return i, s[start+1 : i], true | ||
| } | ||
| } | ||
| } | ||
| return 0, "", false | ||
| } |
There was a problem hiding this comment.
Fixed in follow-up. extractDelimited now tracks nested closing delimiters with a stack, so mixed ${...} inside $(...) expands correctly. Added regression coverage for mixed nesting.
Fix go-to-definition for export/override/conditional variable names, add support for else-if conditional chains, and resolve safe computed include paths for linking into included Makefiles.
owenrumney
force-pushed
the
fix/tidy-up-more-references
branch
from
b19b71a to
0c16b03
Compare
| func preferEarlierVariableDefinition(mf *model.Makefile, current *model.Variable) *model.Variable { | ||
| var best *model.Variable | ||
| for _, v := range mf.Variables { | ||
| if v.Name != current.Name { | ||
| continue | ||
| } | ||
| if v.NameRange.Start.Line > current.NameRange.Start.Line || (v.NameRange.Start.Line == current.NameRange.Start.Line && v.NameRange.Start.Character >= current.NameRange.Start.Character) { | ||
| continue | ||
| } | ||
| best = v | ||
| } | ||
| if best != nil { | ||
| return best | ||
| } | ||
| return current | ||
| } |
There was a problem hiding this comment.
Fixed in follow-up. The earlier-definition preference is now restricted to override assignments and only searches earlier entries in mf.Variables, so merged include vars do not get picked accidentally.
| path := inc.ResolvedPath | ||
| if path == "" { | ||
| path = inc.Path | ||
| } | ||
| return []lsp.Location{{ | ||
| URI: lsp.DocumentURI("file://" + inc.Path), | ||
| URI: lsp.DocumentURI("file://" + path), | ||
| Range: lsp.Range{}, |
There was a problem hiding this comment.
Fixed in follow-up. Include-definition URIs are now constructed via net/url + filepath.ToSlash, so file URIs are valid and properly escaped.
Fix go-to-definition for export/override/conditional variable names, add
support for else-if conditional chains, and resolve safe computed
include paths for linking into included Makefiles.