alecthomas / go-check-sumtype

A simple utility for running exhaustiveness checks on Go "sum types."
The Unlicense
21 stars 3 forks source link

Switch to using analysis framework natively with fact export #5

Open navijation opened 8 months ago

navijation commented 8 months ago

As noted in golangci/golangci-lint#4158, this linter is incompatible with the analysis framework because it strongly relies on state across packages, and this state is not exported using the facts API.

The analyzer shim present in golangci-lint seems insufficient in dealing with this limitation. This entire module would likely need to be re-written as an analyzer.

alecthomas commented 8 months ago

Ah makes sense, I'll take a look.

navijation commented 8 months ago

Thanks for taking a look. As far as in-memory caching goes, my recommendation is the following strategy:

  1. store a cache which maps packages to their own caches; access to this cache must be synchronized since drivers may execute independent packages in parallel
  2. in each package, only perform writes to the cache of the same package and reads from other packages; these caches do not need to be synchronized because no well-behaved driver will perform concurrent passes over dependent packages or multiple passes against the same package
  3. at the beginning of an analysis pass, delete and re-initialize the cache for the package in question to avoid any risk of staleness

Another thing to worry about with using the facts API: once you add analyzer facts, drivers will start running the analyzer against the dependency closure rather than just the packages in question. Sadly, this will drop performance of the singlechecker considerably. There's not really a workaround aside from adding some config option to exclude reporting and fact gathering on certain packages.

alecthomas commented 8 months ago

What caching are you referring to? I've used the facts API before and looking at how it would be used here, it seems sufficient for this use case.

navijation commented 8 months ago

It would be acceptable to use no in memory caching and purely rely on the facts API.