Add RangeMap for unified "in-range" filtering
#3343
Conversation
hls-plugin-api/hls-plugin-api.cabal
Outdated
| @@ -57,6 +58,7 @@ library | |||
| , transformers | |||
| , unordered-containers | |||
| , megaparsec > 9 | |||
| , hw-fingertree | |||
There was a problem hiding this comment.
I'm not keen on adding new dependencies to his-plugin-api. Other alternatives worth considering:
- Extract
RangeMapto a standalone package - Leave it in its current home - plugins can depend on each other just fine
There was a problem hiding this comment.
Although I think the functionality is pretty small to warrant a standalone package, I think I can do that. I am not too keen on the second option. Even if the plugins can depend on each other, I think having this as part of a specific plugin signals the idea that this is something specific to that plugin. I think that may discourage plugin authors to use it (it certainly would discourage me).
What is the downside of adding new dependencies to hls-plugin-api?
There was a problem hiding this comment.
What is the downside of adding new dependencies to hls-plugin-api?
When we update HLS for a new version of GHC, hls-plugin-api is one of the packages that needs to get updated if we are to do anything. We are frequently blocked by upstream dependencies not working with the new version of GHC; every new dependency that we add to key packages is another opportunity for that to happen...
There was a problem hiding this comment.
That said, hw-fingertree depends only on base, which is promising.
There was a problem hiding this comment.
I see. Indeed, I had to add GHC 9.4 support to hw-fingertree myself, so I can definitely see that happening 😅 Then I will extract this into a separate small package.
There was a problem hiding this comment.
Personally I suggest extracting it to a module in hls-plugin-api or something. Having a whole other package is a pain in its own right!
Also, if it would require approximately the same amount of code as is in hw-fingertree then we may not benefit much.
Here's a compromise thought: since RangeMap is exactly supposed to be a faster alternative to a list-based approach, what if we add a cabal flag to hls-plugin-api use-fingertree, such that if it is on, RangeMap is backed by a fingertree, and if it is off, RangeMap is backed by a list. Then we can turn it off for newer GHC versions until we get a working hw-fingertree?
There was a problem hiding this comment.
Personally I suggest extracting it to a module in
hls-plugin-apior something.
I am a bit confused by this. Do you suggest something other than what currently is present in the PR? Because it currently is a module within hls-plugin-api (unless there is a separate concept of module which I'm not aware).
what if we add a cabal flag to
hls-plugin-apiuse-fingertree
Sounds good to me. What do you think @pepeiborra?
There was a problem hiding this comment.
Ah, I think I misunderstood. I thought we were talking about extracting the fingertree code from hw-fingertree so we controlled it, rather than extracting RangeMap. Ignore me, then.
what if we add a cabal flag to hls-plugin-api use-fingertree
... but I still think this is the cleanest solution.
There was a problem hiding this comment.
I have put the fingertree implementation behind a flag. All related tests (tests of RangeMap itself and the plugins that make use of it) pass with both implementations. Can you take another look to see if it reflects what you had in mind? @michaelpj
| @@ -52,7 +54,7 @@ instance NFData CollectLiterals | |||
| type instance RuleResult CollectLiterals = CollectLiteralsResult | |||
|
|
|||
| data CollectLiteralsResult = CLR | |||
| { literals :: [Literal] | |||
| { literals :: RangeMap Literal | |||
| @@ -29,7 +29,7 @@ test :: TestTree | |||
| test = testGroup "alternateNumberFormat" [ | |||
| codeActionHex "TIntDtoH" 3 13 | |||
| , codeActionOctal "TIntDtoO" 3 13 | |||
| , codeActionBinary "TIntDtoB" 4 12 | |||
| , codeActionBinary "TIntDtoB" 4 13 | |||
There was a problem hiding this comment.
Shouldn't cause any worries, it's just the column number to run the code action. I think the column number is arbitrarily in the middle of a number. Looks like it just matches the other test cases.
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
This PR makes
RangeMapa part of the plugin API, first introduced as part ofhls-explicit-record-fields-plugin.RangeMapis a data structure that allows for fast queries of range intersections. Details on usage can be found in the provided documentation, and the performance characteristics compared to the traditional list-based approach can be found in the newly-added benchmark.Browsing through existing plugins, I couldn't find as much opportunities to use this as I hoped to (without significantly modifying plugin source), since to make the best use of this, a plugin needs to:
However, I still think this lays down a nice foundation for future plugins, if they were to need such a functionality.
Screenshot from the output of:
cabal bench hls-plugin-api -O2 --benchmark-options="-o benchmark.html"