search

tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.23k stars 85 forks source link

Fix nls crash, and better refresh diagnostics. #1944

Closed jneem closed 3 weeks ago

jneem commented 4 weeks ago

In nls, if main.ncl imports dep.ncl and dep.ncl changes, we need to invalidate some cached data about main.ncl and re-check it. Previously we were simply resetting main.ncl to the "parsed" state in the cache, but this isn't sufficient because our cached term for main.ncl might depend on dep.ncl. Specifically, import resolution of main.ncl transforms the term in a way that depends on whether dep.ncl parsed.

This PR does the most inefficient (but easiest to get correct) thing: throwing out all the parsed data and re-parsing from scratch. This can be optimized, but it probably isn't too much slower than the status quo, because we were re-typechecking from scratch anyway.

Fixes #1943 and maybe also #1942 and #1935. There are still some bugs related to cache state in the background worker, which I will investigate next.

jneem commented 3 weeks ago

@yannham I've implemented the solution we discussed yesterday, where the full recursive invalidation happens in the cache.

It occurred to me that there's still some possibility for misuse in the cache API, since replace_string deletes the cached term without invalidating things that import it. Maybe replace_string should do the invalidation automatically? I didn't do that here because it's more invasive...

yannham commented 3 weeks ago

It occurred to me that there's still some possibility for misuse in the cache API, since replace_string deletes the cached term without invalidating things that import it. Maybe replace_string should do the invalidation automatically? I didn't do that here because it's more invasive...

Hmm, I'm not entirely set on this. On one hand you're right that this has the same risk of kinda inconsistent state lying around. On the other hand replace_string is used in practice for like queries and error message snippets, which shouldn't be imported from anywhere. Well then maybe it's virtually free to invalidate the reverse dependencies, because in practice there shouldn't be any?

jneem commented 3 weeks ago

Well then maybe it's virtually free to invalidate the reverse dependencies, because in practice there shouldn't be any?

That's probably right, I just didn't want this bugfix to have unintended consequences...