fix: Cache passed type checks for many configs

[nayeemrmn]

Discussed on Discord a couple times.

// temp.ts
console.log("Hello, world!");

// temp.json
{
  "compilerOptions": {
    "lib": [
      "dom",
      "dom.iterable",
      "esnext"
    ]
  }
}

Before:

$ deno check temp.ts
Check file:///home/nayeem/projects/deno/temp.ts
$ deno check -c temp.json temp.ts
Check file:///home/nayeem/projects/deno/temp.ts
$ deno check temp.ts
Check file:///home/nayeem/projects/deno/temp.ts
$ deno check -c temp.json temp.ts
Check file:///home/nayeem/projects/deno/temp.ts

After:

$ deno check temp.ts
Check file:///home/nayeem/projects/deno/temp.ts
$ deno check -c temp.json temp.ts
Check file:///home/nayeem/projects/deno/temp.ts
$ deno check temp.ts
$ deno check -c temp.json temp.ts

[kitsonk]

I will defer to @dsherret as he has been investigating cache validation challenges as a whole.

[dsherret]

I think this breaks a few things, but overall I'm not sure this is worth the complexity especially now that --no-check is the default.

[dsherret]

Will this possibly break stuff like if someone changes their JSX settings? I feel like we should keep config_bytes here.

[nayeemrmn]

Yeah this doesn't factor in config changes which affect emit... but putting that back will break the added behaviour. I mulled over a "correct" structure which looks like this:

#[derive(Debug, Deserialize, Serialize)]
pub struct EmitMetadata {
  #[serde(default)]
  pub source_hash: String,
  /// Hashed config used for the currently stored emit. Only considers options
  /// which affect how code is emitted.
  #[serde(default)]
  pub emit_config_hash: String,
  /// List of hashed configs for successful type checks. Only considers options
  /// which affect the result of a type check.
  #[serde(default)]
  pub check_config_hashes: HashSet<String>,
}

and reached the conclusion that this can be nicely addressed only after we move to full swc emit and decouple emitting and type-checking.

[dsherret]

It makes caching a lot more complicated and I'm still not sure it's worth it. It seems much easier and less error prone to just invalidate all the caches whenever the config changes. How often do people have multiple configs or no config and then a config when type checking? Is this actually something users need?

[nayeemrmn]

It happens with workers also, although that's only with deno run --check now.

[nayeemrmn]

@dsherret I remembered why I wanted to do this, last time it came up was discussing #14632. We could fix shortly fix that just by including the check scope in the config digest. But looking at the cache validation code I strongly think we need better separation between things that bust the emit cache, when a type check needs to be reperformed and tracking if the source has actually changed. As described in #15090 (comment). One of the reasons is that invalid emits from changed sources need to be checked for each module in the graph uniformly, whereas checked state can only correctly be stored at the roots. Another is that some files aren't emitted at all but changes need to be tracked only for type checking.

There have always been odd cache validation bugs and I attribute it to the current structure which conflates invalid emits and yet-to-be-performed type checks and tries to store it all in one version hash. But again I can make a better case and implementation for this after checking and emitting are properly decoupled with swc. Closing for now.

[dsherret]

@nayeemrmn yeah, I think decoupling the two would be really good. I was planning on starting work tomorrow on #13302. Maybe at the same time I will look a bit at decoupling it.

[dsherret]

I looked into this today and the type checking cache seems like it won't be invalidated in more scenarios than mentioned in #14632. I'm updating the code to cache a hash of all the inputs per successful type check which is only cleared when changing CLI versions, so it will solve this issue too.