Skip to content

Commit a373ae3

Browse files
charlieegan3charlieegan3
authored
lsp: Use filename path for current buffer for defns (#1732)
* lsp: Use filename path for current buffer for defns There was a bug here where the defn would only work when the result was another file. Now we use the same format of reference for the current buffer as we use for other modules (a path rather than a URI) Fixes #1700 Signed-off-by: Charlie Egan <charlie_egan@apple.com> * Add helper functions for relative path operations Signed-off-by: Charlie Egan <charlie_egan@apple.com> --------- Signed-off-by: Charlie Egan <charlie_egan@apple.com>
1 parent bf632d0 commit a373ae3

7 files changed

Lines changed: 124 additions & 12 deletions

File tree

‎internal/lsp/eval_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@ import (
55
"os"
66
"path/filepath"
77
"slices"
8-
"strings"
98
"testing"
109

1110
"github.com/google/go-cmp/cmp"
1211

1312
rio "github.com/open-policy-agent/regal/internal/io"
1413
"github.com/open-policy-agent/regal/internal/lsp/log"
14+
"github.com/open-policy-agent/regal/internal/lsp/uri"
1515
rparse "github.com/open-policy-agent/regal/internal/parse"
1616
"github.com/open-policy-agent/regal/internal/testutil"
1717
)
@@ -40,12 +40,12 @@ func TestEvalWorkspacePath(t *testing.T) {
4040
}
4141
`
4242

43-
policy1URI := ls.workspaceRootURI + "/policy1.rego"
44-
policy1RelativeFileName := strings.TrimPrefix(policy1URI, ls.workspaceRootURI+"/")
43+
policy1URI := uri.FromRelativePath(ls.client.Identifier, "policy1.rego", ls.workspaceRootURI)
44+
policy1RelativeFileName := uri.ToRelativePath(ls.client.Identifier, policy1URI, ls.workspaceRootURI)
4545
module1 := testutil.Must(rparse.ModuleWithOpts(policy1RelativeFileName, policy1, rparse.ParserOptions()))(t)
4646

47-
policy2URI := ls.workspaceRootURI + "/policy2.rego"
48-
policy2RelativeFileName := strings.TrimPrefix(policy2URI, ls.workspaceRootURI+"/")
47+
policy2URI := uri.FromRelativePath(ls.client.Identifier, "policy2.rego", ls.workspaceRootURI)
48+
policy2RelativeFileName := uri.ToRelativePath(ls.client.Identifier, policy2URI, ls.workspaceRootURI)
4949
module2 := testutil.Must(rparse.ModuleWithOpts(policy2RelativeFileName, policy2, rparse.ParserOptions()))(t)
5050

5151
ls.cache.SetFileContents(policy1URI, policy1)

‎internal/lsp/lint.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,10 @@ import (
1111
"github.com/open-policy-agent/opa/v1/storage"
1212

1313
"github.com/open-policy-agent/regal/internal/lsp/cache"
14+
"github.com/open-policy-agent/regal/internal/lsp/clients"
1415
"github.com/open-policy-agent/regal/internal/lsp/completions/refs"
1516
"github.com/open-policy-agent/regal/internal/lsp/types"
17+
"github.com/open-policy-agent/regal/internal/lsp/uri"
1618
rparse "github.com/open-policy-agent/regal/internal/parse"
1719
"github.com/open-policy-agent/regal/internal/util"
1820
"github.com/open-policy-agent/regal/pkg/config"
@@ -46,6 +48,7 @@ type updateParseOpts struct {
4648
Builtins map[string]*ast.Builtin
4749
RegoVersion ast.RegoVersion
4850
WorkspaceRootURI string
51+
ClientIdentifier clients.Identifier
4952
}
5053

5154
// updateParse updates the module cache with the latest parse result for a given URI,
@@ -61,7 +64,7 @@ func updateParse(ctx context.Context, opts updateParseOpts) (bool, error) {
6164
options := rparse.ParserOptions()
6265
options.RegoVersion = opts.RegoVersion
6366

64-
presentedFileName := strings.TrimPrefix(opts.FileURI, opts.WorkspaceRootURI+"/")
67+
presentedFileName := uri.ToRelativePath(opts.ClientIdentifier, opts.FileURI, opts.WorkspaceRootURI)
6568

6669
module, err := rparse.ModuleWithOpts(presentedFileName, content, options)
6770
if err == nil {

‎internal/lsp/lint_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"github.com/open-policy-agent/opa/v1/ast"
88

99
"github.com/open-policy-agent/regal/internal/lsp/cache"
10+
"github.com/open-policy-agent/regal/internal/lsp/clients"
1011
"github.com/open-policy-agent/regal/internal/lsp/types"
1112
"github.com/open-policy-agent/regal/internal/parse"
1213
"github.com/open-policy-agent/regal/internal/testutil"
@@ -109,6 +110,7 @@ allow[msg] { 1 == 1; msg := "hello" }
109110
Builtins: ast.BuiltinMap,
110111
RegoVersion: testData.regoVersion,
111112
WorkspaceRootURI: "",
113+
ClientIdentifier: clients.IdentifierGeneric,
112114
}))(t)
113115

114116
if success != testData.expectSuccess {

‎internal/lsp/server.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1510,7 +1510,7 @@ func (l *LanguageServer) handleTextDocumentDefinition(params types.DefinitionPar
15101510

15111511
query := oracle.DefinitionQuery{
15121512
// The value of Filename is used if the defn in the current buffer.
1513-
Filename: params.TextDocument.URI,
1513+
Filename: l.toRelativePath(params.TextDocument.URI),
15141514
Pos: positionToOffset(contents, params.Position),
15151515
Modules: modules,
15161516
Buffer: outil.StringToByteSlice(contents),
@@ -1531,7 +1531,7 @@ func (l *LanguageServer) handleTextDocumentDefinition(params types.DefinitionPar
15311531
return types.Location{
15321532
// res.File will be relative to the workspace root. The response here needs
15331533
// a URI for the client to be able to navigate correctly.
1534-
URI: util.EnsureSuffix(l.workspaceRootURI, "/") + res.File,
1534+
URI: uri.FromRelativePath(l.client.Identifier, res.File, l.workspaceRootURI),
15351535
Range: types.RangeBetween(res.Row-1, res.Col-1, res.Row-1, res.Col-1),
15361536
}, nil
15371537
}
@@ -2284,7 +2284,7 @@ func (l *LanguageServer) toPath(fileURI string) string {
22842284
}
22852285

22862286
func (l *LanguageServer) toRelativePath(fileURI string) string {
2287-
return strings.TrimPrefix(l.toPath(fileURI), l.workspacePath()+string(os.PathSeparator))
2287+
return uri.ToRelativePath(l.client.Identifier, fileURI, l.workspaceRootURI)
22882288
}
22892289

22902290
func (l *LanguageServer) fromPath(filePath string) string {
@@ -2323,6 +2323,7 @@ func (l *LanguageServer) parseOpts(fileURI string, bis map[string]*ast.Builtin)
23232323
Builtins: bis,
23242324
RegoVersion: l.regoVersionForURI(fileURI),
23252325
WorkspaceRootURI: l.workspaceRootURI,
2326+
ClientIdentifier: l.client.Identifier,
23262327
}
23272328
}
23282329

‎internal/lsp/server_rename_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func TestLanguageServerFixRenameParams(t *testing.T) {
3232
}},
3333
}
3434

35-
fileURI := ls.workspaceRootURI + "/foo/bar/policy.rego"
35+
fileURI := uri.FromRelativePath(ls.client.Identifier, "foo/bar/policy.rego", ls.workspaceRootURI)
3636
ls.cache.SetFileContents(fileURI, "package authz.main.rules")
3737

3838
params := testutil.Must(ls.fixRenameParams("fix my file!", fileURI))(t)
@@ -75,7 +75,7 @@ func TestLanguageServerFixRenameParamsWithConflict(t *testing.T) {
7575
}},
7676
}
7777

78-
fileURI := ls.workspaceRootURI + "/foo/bar/policy.rego"
78+
fileURI := uri.FromRelativePath(ls.client.Identifier, "foo/bar/policy.rego", ls.workspaceRootURI)
7979
conflictingFileURI := fmt.Sprintf("file://%s/workspace/authz/main/rules/policy.rego", tmpDir)
8080

8181
ls.cache.SetFileContents(fileURI, "package authz.main.rules")
@@ -148,7 +148,7 @@ func TestLanguageServerFixRenameParamsWhenTargetOutsideRoot(t *testing.T) {
148148
}},
149149
}
150150

151-
fileURI := ls.workspaceRootURI + "foo/bar/policy.rego"
151+
fileURI := uri.FromRelativePath(ls.client.Identifier, "foo/bar/policy.rego", ls.workspaceRootURI)
152152
ls.cache.SetFileContents(fileURI, "package authz.main.rules")
153153

154154
_, err := ls.fixRenameParams("fix my file!", fileURI)

‎internal/lsp/uri/uri.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"strings"
88

99
"github.com/open-policy-agent/regal/internal/lsp/clients"
10+
"github.com/open-policy-agent/regal/internal/util"
1011
)
1112

1213
// uris always use / as the uriSeparator, regardless of system.
@@ -59,3 +60,24 @@ func ToPath(client clients.Identifier, uri string) string {
5960
// Convert path to use system separators
6061
return filepath.FromSlash(path)
6162
}
63+
64+
// ToRelativePath converts a URI to a file path relative to the given workspace root URI.
65+
func ToRelativePath(client clients.Identifier, uri, workspaceRootURI string) string {
66+
absolutePath := ToPath(client, uri)
67+
workspaceRootPath := ToPath(client, workspaceRootURI)
68+
69+
// Ensure workspace root path has trailing separator for consistent trimming
70+
if workspaceRootPath != "" {
71+
workspaceRootPath = util.EnsureSuffix(workspaceRootPath, "/")
72+
}
73+
74+
return strings.TrimPrefix(absolutePath, workspaceRootPath)
75+
}
76+
77+
// FromRelativePath creates a URI from a relative path and workspace root URI.
78+
func FromRelativePath(client clients.Identifier, relativePath, workspaceRootURI string) string {
79+
workspaceRootPath := ToPath(client, workspaceRootURI)
80+
absolutePath := filepath.Join(workspaceRootPath, relativePath)
81+
82+
return FromPath(client, absolutePath)
83+
}

‎internal/lsp/uri/uri_test.go‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,3 +173,87 @@ func TestURIToPath_VSCode(t *testing.T) {
173173
})
174174
}
175175
}
176+
177+
func TestToRelativePath(t *testing.T) {
178+
t.Parallel()
179+
180+
testCases := map[string]struct {
181+
uri string
182+
workspaceRootURI string
183+
want string
184+
}{
185+
"unix simple": {
186+
uri: "file:///workspace/foo/bar.rego",
187+
workspaceRootURI: "file:///workspace",
188+
want: "foo/bar.rego",
189+
},
190+
"unix with trailing slash in workspace": {
191+
uri: "file:///workspace/foo/bar.rego",
192+
workspaceRootURI: "file:///workspace/",
193+
want: "foo/bar.rego",
194+
},
195+
"windows": {
196+
uri: "file:///c:/workspace/foo/bar.rego",
197+
workspaceRootURI: "file:///c:/workspace",
198+
want: filepath.FromSlash("foo/bar.rego"),
199+
},
200+
"root path": {
201+
uri: "file:///workspace/policy.rego",
202+
workspaceRootURI: "file:///workspace",
203+
want: "policy.rego",
204+
},
205+
}
206+
207+
for label, tc := range testCases {
208+
t.Run(label, func(t *testing.T) {
209+
t.Parallel()
210+
211+
got := ToRelativePath(clients.IdentifierGeneric, tc.uri, tc.workspaceRootURI)
212+
if got != tc.want {
213+
t.Errorf("got %q, want %q", got, tc.want)
214+
}
215+
})
216+
}
217+
}
218+
219+
func TestFromRelativePath(t *testing.T) {
220+
t.Parallel()
221+
222+
testCases := map[string]struct {
223+
relativePath string
224+
workspaceRootURI string
225+
want string
226+
}{
227+
"unix simple": {
228+
relativePath: "foo/bar.rego",
229+
workspaceRootURI: "file:///workspace",
230+
want: "file:///workspace/foo/bar.rego",
231+
},
232+
"unix with trailing slash in workspace": {
233+
relativePath: "foo/bar.rego",
234+
workspaceRootURI: "file:///workspace/",
235+
want: "file:///workspace/foo/bar.rego",
236+
},
237+
"windows": {
238+
relativePath: "foo/bar.rego",
239+
workspaceRootURI: "file:///c:/workspace",
240+
want: "file:///c:/workspace/foo/bar.rego",
241+
},
242+
"root file": {
243+
relativePath: "policy.rego",
244+
workspaceRootURI: "file:///workspace/",
245+
want: "file:///workspace/policy.rego",
246+
},
247+
}
248+
249+
for label, tc := range testCases {
250+
t.Run(label, func(t *testing.T) {
251+
t.Parallel()
252+
253+
got := FromRelativePath(clients.IdentifierGeneric, tc.relativePath, tc.workspaceRootURI)
254+
if got != tc.want {
255+
t.Errorf("got %q, want %q", got, tc.want)
256+
}
257+
})
258+
}
259+
}

0 commit comments

Comments
 (0)