Typed rules - #5025
Conversation
wz1000
left a comment
There was a problem hiding this comment.
Broadly looks good, some inputs need to be tightened up
fendor
left a comment
There was a problem hiding this comment.
Thanks, this looks very nice! This is a nitpick review, so it is the intention to be done after the review is addressed. I have reviewed the ghcide changes, will review plugins soon.
Further, we agreed the following documentation is still missing and will be added:
* What are the existing rules?
* What is RuleInput?
* Why do we have RuleInput
* What is the design of RuleInput
Note [Hierarchical Inputs]
Note [RuleInput]
Reference these notes from various type classes and RuleInput type
* Helper functions need documentation
| toPriority :: PluginError -> Priority | ||
| toPriority (PluginInternalError _) = Error | ||
| toPriority (PluginInvalidParams _) = Warning | ||
| toPriority (PluginUnsupportedUriType _) = Warning |
There was a problem hiding this comment.
We might want to demote this to Debug. IIRC, a warning is displayed in the editor, but these errors will be perfectly sensible in the future, right?
@wz1000 opinions?
144b524 to
ec6e677
Compare
bc09427 to
0afd603
Compare
| case file of | ||
| SomeProjectHaskellInput projectFile -> | ||
| setFileModified (cmapWithPrio LogFileStore recorder) (VFSModified vfs) ide False projectFile action | ||
| _ -> setSomethingModified (VFSModified vfs) ide (fromNormalizedFilePath (inputFilePath file) ++ " (modified)") action |
There was a problem hiding this comment.
We shouldn't do anything here, rather log this case.
There was a problem hiding this comment.
@wz1000 What should we do in this case? Call setSomethingModified for completeness? We need to add it to the set of files of interest, but it might not need to record that it isn't the first time this non project haskell file was opened, right? 🤔
There was a problem hiding this comment.
setSomethingModified is not correct here, that will trash the entire session. I think setFileModified should accept SomeHaskellInput instead of ProjectHaskellInput
It should always restart with the new VFS and dirty the file’s GetModificationTime, then run typecheckParents only in the SomeProjectHaskellInput branch
| case file of | ||
| SomeProjectHaskellInput projectFile -> | ||
| setFileModified (cmapWithPrio LogFileStore recorder) (VFSModified vfs) ide True projectFile action | ||
| _ -> setSomethingModified (VFSModified vfs) ide (fromNormalizedFilePath (inputFilePath file) ++ " (modified)") action |
wz1000
left a comment
There was a problem hiding this comment.
plugins and input types broadly look correct. I need to do another more careful pass though.
| case file of | ||
| SomeProjectHaskellInput projectFile -> | ||
| setFileModified (cmapWithPrio LogFileStore recorder) (VFSModified vfs) ide False projectFile action | ||
| _ -> setSomethingModified (VFSModified vfs) ide (fromNormalizedFilePath (inputFilePath file) ++ " (modified)") action |
There was a problem hiding this comment.
setSomethingModified is not correct here, that will trash the entire session. I think setFileModified should accept SomeHaskellInput instead of ProjectHaskellInput
It should always restart with the new VFS and dirty the file’s GetModificationTime, then run typecheckParents only in the SomeProjectHaskellInput branch
ab25f1d to
243e8a9
Compare
Introduces typed rule inputs so each HLS rule explicitly declares the kinds of input it can consume. This replaces the previous NormalizedFilePath (NFP)-based convention with a typed model in which inputs are classified before a rule is invoked. The hierarchy captures relationships between global, file-based, Haskell, Cabal, project, and dependency inputs. Broader rule-input types do not automatically subsume more specific inputs, so callers must explicitly pattern match to narrow a general input or wrap a specific input to widen it to the exact type expected by a rule. This allows the compiler to reject invalid rule invocations and makes rule scope, dependencies, caching, and invalidation behavior clearer.
This PR introduces typed inputs for HLS/ghcide rules.
HLS already associates every rule key with its result through
RuleResult, but all rules previously accepted the same input type:NormalizedFilePath. Theis PR adds an open type family that associates each rule with the kind of input it supports:Rule inputs are now part of the rule's contract, just like rule results.
Why this is needed
A raw
NormalizedFilePathdoes not tell us what a path actually represent. These distinctions matter. For example,TypeCheckandGhcSessionrequire a project component and session, while file-content rules can work on many filetypes. Typed inputs make these assumptions explicit, reject incorrect rule/input pairings earlier, and provide the foundation needed to support dependency sources without treating them as project modules.
How it works
The new
Development.IDE.Core.RuleInputmodule defines this input hierarchy:Internally, Shake still needs one heterogeneous key representation.
SomeInputtherefore existentially stores typed inputs inside
Q, whileInputFingerprintprovides consistent equality and hashing.