feat: update DevContext and LSP to support composable schemas#2965
feat: update DevContext and LSP to support composable schemas#2965miparnisari merged 2 commits intomainfrom
Conversation
cffe476 to
e20db9f
Compare
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (74.92%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2965 +/- ##
==========================================
+ Coverage 74.79% 74.92% +0.14%
==========================================
Files 499 500 +1
Lines 61065 61229 +164
==========================================
+ Hits 45666 45869 +203
+ Misses 12200 12158 -42
- Partials 3199 3202 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3998ee0 to
e20db9f
Compare
9ef3b65 to
5dcc950
Compare
12bc73d to
a1130f6
Compare
254aa41 to
10d5e84
Compare
| Context: errWithSource.SourceCodeString, | ||
| Line: lineNumber, | ||
| Column: columnPosition, | ||
| Path: []string{path}, |
There was a problem hiding this comment.
FYI zed uses this to show the correct content from the file that has the error
fe72591 to
dae057f
Compare
|
|
||
| // Get errors. | ||
| for _, devErr := range devErrs.GetInputErrors() { | ||
| if !slices.Contains(devErr.Path, string(uri)) { |
There was a problem hiding this comment.
If I recall correctly, I had a map somewhere from URI <-> Path; if not, we likely should add it or helper functions, to make it clear when using one vs the other
| targetURI := params.TextDocument.URI | ||
| if resolved.TargetSource != nil && *resolved.TargetSource != "schema" { | ||
| sourceDir := uriToSourceDir(params.TextDocument.URI) | ||
| targetURI = lsp.DocumentURI("file://" + filepath.Join(sourceDir, string(*resolved.TargetSource))) |
There was a problem hiding this comment.
And you'd use the new map/mapping functions here
| return nil, err | ||
| } | ||
|
|
||
| targetSource := r.resolveTargetSource(relation.Name, source) |
There was a problem hiding this comment.
FYI this is what VSCode uses to know which file contains the error and display the right text content
| // DefinitionNodeSource returns the input source for the AST node defining the given name. | ||
| // For definitions compiled from imports, this will be the imported file path (e.g. "users.zed"). | ||
| // For definitions in the root schema, this will be the root source (e.g. "schema"). |
There was a problem hiding this comment.
If there is a better way to do this please let me know
| nodeSource, _ := toplevel.GetString(dslshape.NodePredicateSource) | ||
| errMsg := fmt.Errorf("failed to read import %q: %w", filePath, err) | ||
| return nil, "", toContextError(errMsg.Error(), nodeSource, toplevel, mapper) |
There was a problem hiding this comment.
FYI changing to contextError so that we don't lose line number info of where the error happened
dae057f to
36b1c7e
Compare
f4798a6 to
2f18c9e
Compare
2f18c9e to
79b1c89
Compare
…d support for textDocument/definition, support composable schemas). fix position mapper
79b1c89 to
e09bfdd
Compare
| // resolveURI resolves a relative path against the directory of baseURI, | ||
| // returning a new file:// DocumentURI. | ||
| func resolveURI(baseURI lsp.DocumentURI, relativePath string) lsp.DocumentURI { | ||
| return lsp.DocumentURI("file://" + filepath.Join(uriToSourceDir(baseURI), relativePath)) |
There was a problem hiding this comment.
Could use net/url here by replacing Scheme: https://stackoverflow.com/questions/63197536/replacing-protocol-and-hostname-in-url-in-go
| // WithRootFileName saves the root file of the schema | ||
| // so errors can be displayed accurately on UIs. | ||
| func WithRootFileName(name string) CompileOption { | ||
| return func(cfg *compileConfig) { cfg.rootFileName = name } | ||
| } |
There was a problem hiding this comment.
Barak and I were talking about this - would it make sense to hang the filename off of the definition such that error handling can look up the filename along with the position straight off the node?
There was a problem hiding this comment.
I'll do this in a follow-on PR
| func (r *SchemaPositionMapper) resolveTargetSource(name string, fallback input.Source) input.Source { | ||
| if defSource := r.schema.GetPathToDefinitionOrPartialOrCaveat(name); defSource != "" { | ||
| return input.Source(defSource) | ||
| } | ||
| return fallback | ||
| } |
There was a problem hiding this comment.
Yeah, I think that would clean up this code a bit
| sourceFS := fstest.MapFS{ | ||
| "path/partials.zed": &fstest.MapFile{Data: []byte("use partial\npartial secret {\nrelation secret: user\n}")}, | ||
| "path/users.zed": &fstest.MapFile{Data: []byte("definition user {}\ncaveat is_raining(day string) {\nday == \"saturday\"\n}")}, | ||
| } |
There was a problem hiding this comment.
Oooh I like this
| // GetPathToDefinitionOrPartialOrCaveat returns the input source for the AST node defining the given name. | ||
| // For definitions compiled from imports, this will be the imported file path (e.g. "users.zed"). | ||
| // For definitions in the root schema, this will be the root source (e.g. "schema"). | ||
| // Returns empty string if not found. | ||
| func (cs *CompiledSchema) GetPathToDefinitionOrPartialOrCaveat(name string) string { |
There was a problem hiding this comment.
Yeah, I think we should annotate the node rather than treating it as a calculated value. I can make that change today.
There was a problem hiding this comment.
Deferring to future refactor
| // Skip nodes from imported files whose rune positions may overlap with the root file. | ||
| if nodeSource, err := node.GetString(dslshape.NodePredicateSource); err == nil { | ||
| if nodeSource != rootSource { | ||
| return nil, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this part necessary?
There was a problem hiding this comment.
Makes this test pass: "cursor on the import path" on schema_position_mapper_test.go
|
|
||
| // Take a filepath and ensure that it's local to the current context. | ||
| func validateFilepath(path string) error { | ||
| func validateFilepath(path string, toplevel *dslNode, mapper input.PositionMapper) error { |
There was a problem hiding this comment.
I think we might be able to delegate this to DirFS's restrictions. I guess we'd still need to rewrite the error in that case, though.
Description
Filesystem-aware development system — The DevContext and schema compilation now accept a filesystem
(fs.FS), enabling validation of schemas that use import syntax (composable schemas).
Key changes:
pkg/development/— devcontext.go and schema.go updated to thread an fs.FS through schemacompilation. The position mapper (
schema_position_mapper.go) was reworked significantly tohandle multi-file schemas correctly, with new tests.
pkg/schemadsl/compiler/— The importer and position mapper now work with filesystem-backedsources. development.go gained helpers for filesystem-aware compilation. The translator
tracks source file information more precisely.
LSP (internal/lsp/)— New overlay filesystem (overlay.go) lets the LSP validate open/unsaved filesby layering in-memory content over the real filesystem. Handlers updated to support
textDocument/definition(go-to-definition) and fixrequestDiagnostics. Substantial new tests.Testing
you can test this by bringing it over to VSCode and trying it out with schemas of your own. you have to
gh pr checkout 2965 && mage build:binarygh pr checkout 51package.jsonto point to the binary you created in step 1if you want to test in zed:
gh pr checkout 635 && mage build:binary./zed validate ....