HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.04k stars 647 forks source link

Diagnostics sucks #8949

Closed Simn closed 4 years ago

Simn commented 4 years ago

We have been plagued by diagnostics for quite a while now and it's about time to do something about it. The core issue is that we implement diagnostics by typing the module and picking up various information from that process. This means that we invalidate modules whenever we want diagnostics for them, because if we don't invalidate them we don't type them either.

My original idea was to re-cache diagnostics modules after diagnosing them, but this runs into all the problems encountered in #8596.

What we should do instead is make sure that we persist all the data necessary to diagnose. This way, we can implement diagnostics with a basic branching approach:

  1. If the module is cached, analyze the typed AST and report diagnostics.
  2. Otherwise, type it normally and collect potential errors.
    1. If there are errors, show them as diagnostics errors.
    2. Otherwise, take the typed module and analyze the typed AST.

I've already dealt with quite a bit of related things in #5696. There's at least two remaining problems:

Import positions

At the moment, these live in the common context as this:

mutable import_positions : (pos,bool ref * placed_name list) PMap.t;

The bool ref is set to true if an import is used to resolve a type, which is then checked to detect unused imports (this includes using too). Since imports are local to modules anyway, it should be a simple change to store this data in module_def_display instead, which is then persisted in the compilation cache. This way, the typed AST analyzer can report unused imports easily.

One potential problem is mixed macro/non-macro code. I'll have to think about this a bit.

Dead conditional compilation blocks

Similar story, they also reside in the common context:

mutable dead_blocks : (string,(pos * expr) list) Hashtbl.t;

The string key here is the file path. This means that it's not quite module-based because we intentionally mix macro and non-macro modules. A block is only considered dead if it is dead in both cases.


I would like to resolve this for 4.1 because #8939 isn't very useful otherwise.

Gama11 commented 4 years ago

https://github.com/HaxeFoundation/haxe/issues/8670 seems related too, might want to make sure that data is going to fit in nicely with the new approach as well.

Simn commented 4 years ago

The macro/non-macro situation is a bit tricky. What we want here is the intersection of the information, no the sum: A block is only dead if it is dead in both macro and non-macro. An import is only unused if it is unused in both macro and non-macro.

The issue is that macro and non-macro modules don't know each other, so it's unclear how to model the data here. One way would be to move it to the modules like suggested, then intersect it by making sure both versions of the module are loaded.

Simn commented 4 years ago

I've moved import positions to modules. The dead block handling is still a TODO.

Simn commented 4 years ago

After merging #9132 diagnostics sucks less now.