HaxeFoundation / HaxeFoundation/haxe

Rethinking display modes

Open
#9,130 3 comments 5 reactions 1 assignee Claimed by @Simn View on GitHub
discussion feature-compiler-cache feature-ide strong-belgian-ale
Dominant language
Haxe
Stars
6.9k
Forks
715
Avg merge
2d 2h
Merged PRs (30d)
11

Description

Our current approach to handling display modes needs an update. The implementation has grown from the original dot-completion-only to a substantial framework for IDE support. Now that we have some more insight into the requirements and challenges, it is time to redesign some aspects of our internal implementation.

## Compilation server

The central challenge here is designing the compilation server correctly. We want to use it as a database which caches the latest state of our types. This requires figuring out how to handle partial updates properly.

The original approach was to cache everything on compilation and then access the information when requesting completion. This is, however, impractical because it frequently invalidates changing modules and, by extension, their dependent modules.

## Partial typing

The compiler has various checks in place to make completion in particular very fast. It minimizes the amount of typing on completion requests in order to reach the point of completion as quickly as possible. This is a necessity for a good IDE experience, but we never properly dealt with the effects this has on the compilation server state.

## Type name X redefined from module X

Almost any attempt to improve compilation server robustness against partial typing ultimately ran into the infamous `Type name X redefined from module X` error. The core issue here is that we keep hard references to modules in our cached AST. This means that any module, even partially typed ones, may end up being referenced in the cached data.

Unfortunately, at this point it is not feasible to redesign our typed AST accordingly. Hard references can creep in virtually anywhere given that types are part of `TType.t`, which is the data structure we use to associate types with expressions. Through type inference, a module that we're partially typing could be bound to a monomorph in an entirely different module. But even with explicit type references, resolution could pick up such a module.

## Grouping display modes

Before we continue thinking about this, it is helpful to look at what kind of display modes we actually have. This is the current definition of `DisplayMode.t`:

```ocaml
type t =
| DMNone
| DMDefault
| DMUsage of bool (* true = also report definition *)
| DMDefinition
| DMTypeDefinition
| DMImplementation
| DMResolve of string
| DMPackage
| DMHover
| DMModuleSymbols of string option
| DMDiagnostics of bool (* true = global, false = only in display file *)
| DMStatistics
| DMSignature
```

Let's try grouping these by "difficulty":

* `DMNone` is normal compilation, there is nothing to do here.
* `DMResolve` is probably something we can just remove because it is likely unused.
* `DMPackage` and `DMModuleSymbols None` (= document symbols) are purely syntactical. The only information they need is the module file path and the module syntax, respectively.
* `DMModuleSymbols (Some _)` (= workspace symbols) requires global syntactic information.
* `DMDiagnostics false` (= local diagnostics) _should_ only require local information (this is what I'm working on in #8949).
* `DMDiagnostics true` (= global diagnostics) should be the same but for all known modules. Here it is likely fine to not deal with uncached modules.
* `DMStatistics` (= code lens) requires global information. It might have to consider uncached information for all symbols in the module (#9094).
* `DMDefinition`, `DMHover` and `DMTypeDefinition` only need to know what exactly we're interested in when making a request. The data is then locally available. The original syntax is often intact (e.g. you hover without changing the file contents).
* `DMSignature` is similar, but the display file can be assumed to be in a transient state.
* `DMDefault` (= completion) is again similar, but requires global information in the case of toplevel-completion.
* `DMUsage` (= find references) and `DMImplementation` need to identify what we're interested in first and then require global information. They might have to consider uncached information for the symbol.

This gives us the following grouping:

* syntactic: `DMPackage`, `DMModuleSymbols`
* per-module typed: `DMDiagnostics`
* global typed with name-based exploration: `DMStatistics`
* find-in-syntax with local type information: `DMDefinition`, `DMHover`, `DMTypeDefinition`, `DMSignature`, `DMDefault` (for dot completion)
* find-in-syntax with global type information: `DMDefault` (for toplevel completion)
* find-in-syntax with global type information and name-based exploration: `DMUsage`, `DMImplementation`

## Conclusions

* We shouldn't invalidate any cached information unless the module file physically changes, which we get notified about by the IDE via a `"server/invalidate"` request.
* We should separate the handling of what/where we want to display from real typing. The main reason we invalidate cached files and modules right now is so that we can inject `EDisplay` nodes, but in many cases that's not necessary.
* It's still not clear how we can avoid slowly losing more and more modules from the cache if their files are being modified. We can experiment with recompiling as a background task (#8734), but it's generally quite tricky to update a cache.
* The cached AST should retain all information that we need for display purposes. In particular, this means that we should try to avoid early inlining as that causes many problems.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.