[WIP] Source cache refactor #1827
Conversation
There was a problem hiding this comment.
I didn't look into all the details. I think I like the general idea of making the cache...just a cache. Although the previous one had a nice encapsulation, it also made extracting some data from it (like source ids or the like) quite painful, needing to couple code with the actual cache instead of ImportResolver for example.
One aspect that needs to be really considered is the error tolerance, though. I see it disappeared from the new cache. I think we had several series of annoying bugs where some operation should be error-tolerant but wasn't, or the converse, leading to errors being reported too late in the pipe line. We thus decided to set in stone at the cache level if we are in an error tolerant mode (basically, LSP), or not. Doing that on a per-function basis was just too error prone. I think we should have the same kind of guarantee in the new cache (or it can somewhere else than in cache, but at least centralized in an object such that each individual "parse" doesn't have to think about error tolerance). I honestly don't know these days how the LSP uses the cache. Maybe it's also possible to make the cache strict by default, with specific method for error-tolerant parsing, and use that only in the LSP. Although I think that's exactly what we used to do, and it was still not entirely reliable.
core/src/parser/mod.rs
Outdated
| #[cfg(feature = "nix-experimental")] | ||
| Nix, | ||
| Raw, | ||
| } |
There was a problem hiding this comment.
I wonder if we should mix that with this module. In particular because it's been in the back of my head to move the (pure) Nickel parser in a separate crate, so that
- It can be used by other crates to make tooling around Nickel without depending on the whole of nickel-lang-core
- It's a separate compilation unit that we don't need to even look at if we didn't touch the grammar
I guess it's still doable, but we should be wary of coupling too much the "parser pure Nickel code" with "parse generic Nickel inputs including JSON", if possible.
There was a problem hiding this comment.
Those are good points. I'm thinking of renaming the prepare.rs module into a driver module where this functionality might fit better. That should also give us a better point to track error tolerance.
lsp/nls/src/files.rs
Outdated
| file_id: FileId, | ||
| ) -> Result<CacheOp<()>, Vec<Diagnostic<FileId>>> { | ||
| file_id: CacheKey, | ||
| ) -> Result<(), Vec<Diagnostic<CacheKey>>> { |
There was a problem hiding this comment.
I vaguely remember CacheOp<()> being a useful distinction from () to avoid cycles/loops when typechecking or importing circular stuff. But I'm not sure.
There was a problem hiding this comment.
Grepping through the source code, by now CacheOp is used in a single place in the LSP (apart from the cache itself) and that is an extension function to the cache. So I think it's safe to get rid of it in the external interface of the cache.
core/src/prepare.rs
Outdated
| } | ||
| _ => Ok(cache | ||
| .get_parse_errors(cache_key) | ||
| .expect("Any parsed entry should have a corresponding entry in parse_errors") |
There was a problem hiding this comment.
Should we encode that with an enum, instead of a separate EntryState in a struct?
There was a problem hiding this comment.
I'm not sure why I wanted the parse errors to be stored separately from the cache entries, but that seems silly to me now. I've changed the code to store parse errors directly in the cache entry for parsed entries. The only wrinkle is that it would be annoying to keep parse errors around after the entry is advanced from "parsed" to "typechecked". But would there be any reason to proceed to typechecking for a source file that doesn't parse correctly, even in the LSP?
There was a problem hiding this comment.
Ah, I believe we do proceed with typechecking on a source file that doesn't parse correctly. If only because typechecking is actually the driver for the whole code analysis (goto(ref|def), hover, etc.), and we want to provide analysis for as much code as possible. In fact the typechecker explicitly ignores parse errors in the AST for that very reason:
nickel/core/src/typecheck/mod.rs
Line 1797 in baf6e3d
No description provided.