Conversation
|
d9a29da to
a5d6b61
Compare
a5d6b61 to
4c088e6
Compare
carljm
left a comment
There was a problem hiding this comment.
Thank you, this looks great to me! Even though it doesn't solve the problem of reporting diagnostics across the Type / TypeInferenceBuilder boundary, because of needing a node/location, it still looks like a clear improvement in API, and the DropBomb will help prevent dropping diagnostics by accident.
| db: &'db dyn Db, | ||
| file: File, | ||
| diagnostics: std::cell::RefCell<TypeCheckDiagnostics>, | ||
| defused: DropBomb, |
There was a problem hiding this comment.
minor nit: the name defused confused me at first, because initially this is not defused! It's only defused after finish(). Would have been clearer to me if it were named bomb or drop_bomb
I feel like the direction we were heading in the discussion this morning is to continue returning outcome/result types from I think this direction makes sense. I don't want to be passing, or to require, nodes/locations everywhere in Given that approach, I don't think it's too hard to understand where to pass Db vs InferContext. |
4c088e6 to
07820ee
Compare
Summary
I'm currently on the fence about landing the #14760 PR because it's unclear how we'd support tracking used and unused suppression comments in a performant way:
unused_suppression_comments(db, file)would re-run on every incremental change and for every file. I don't expect the operation itself to be expensive, but it all adds up in a project with 100k+ filesBecause of that, I want to wait to adopt salsa accumulators for type check diagnostics (we could start using them for other diagnostics) until we have very specific reasons that justify regressing incremental check performance.
This PR does a "small" refactor that brings us closer to what I have in #14760 but without using accumulators. To emit a diagnostic, a method needs:
This PR introduces a new
InferContextthat holds on to the db, the current file, and the reported diagnostics. It replaces theTypeCheckDiagnosticsBuilder. We pass theInferContextinstead of thedbto methods that might emit diagnostics. This simplifies some of theOutcomemethods, which can now be called with a context instead of adband the diagnostics builder. Having thedband the file on a single type like this would also be useful when using accumulators.This PR doesn't solve the issue that the
Outcometypes feel somewhat complicated nor that it can be annoying when you need to report aDiagnostic,but you don't have access to anInferContext(or the file). However, I also believe that accumulators won't solve these problems because:InferContext.HasTytrait (e.g., a linter) don't want to bother getting theFilewhen callingType::return_tybecause they aren't interested in the created diagnostics. They just want to know what calling the current expression would return (and if it even is a callable). This is what the different methods ofOutcomeenable today. I can ask for the return type without needing extra data that's only relevant for emitting a diagnostic.A shortcoming of this approach is that it is now a bit confusing when to pass
dband when anInferContext. An option is that we'd make thefileonInferContextoptional (it won't collect any diagnostics ifNone) and change all methods onTypeto takeInferContextas the first argument instead of adb. I'm interested in your opinion on this.Accumulators are definitely harder to use incorrectly because they remove the need to merge the diagnostics explicitly and there's no risk that we accidentally merge the diagnostics twice, resulting in duplicated diagnostics. I still value performance more over making our life slightly easier.