Fix error in code range #3229
Fix error in code range #3229
Conversation
There was a problem hiding this comment.
What about keeping the structure of the current code, but using ExceptT over a custom error type. Then we can fail saying "couldn't get a dependency" or whatever, and then decide just below pluginResponse what to do about that, e.g. log or return an error or what.
(Generally, I think it would be nice if pluginResponse encouraged people to do this instead of just using a String as the error type and turning them all into response errors...)
Good idea! |
| showError :: SelectionRangeError -> Maybe String | ||
| -- This might happen if the HieAst is not ready, so we give it a default value instead of throwing an error | ||
| showError SelectionRangeBadDependency = Nothing | ||
| showError SelectionRangeInputPositionMappingFailure = Just "failed to apply position mapping to input positions" | ||
| showError SelectionRangeOutputPositionMappingFailure = Just "failed to apply position mapping to output positions" | ||
|
|
||
| data SelectionRangeError = SelectionRangeBadDependency | ||
| | SelectionRangeInputPositionMappingFailure | ||
| | SelectionRangeOutputPositionMappingFailure | ||
|
|
||
| getSelectionRanges :: NormalizedFilePath -> [Position] -> ExceptT SelectionRangeError IdeAction [SelectionRange] |
There was a problem hiding this comment.
@michaelpj Did I get it correctly?
| Right list -> Right list | ||
|
|
||
| showError :: SelectionRangeError -> Maybe String | ||
| -- This might happen if the HieAst is not ready, so we give it a default value instead of throwing an error |
There was a problem hiding this comment.
I'd probably just inline this function, then it would look less weird that you're kind of doing different things in the different branches?
| getSelectionRanges file positions = do | ||
| (codeRange, positionMapping) <- maybeToExceptT "fail to get code range" $ useE GetCodeRange file | ||
| (codeRange, positionMapping) <- maybeToExceptT SelectionRangeBadDependency . MaybeT $ | ||
| useWithStaleFast GetCodeRange file |
There was a problem hiding this comment.
maybe we should include the dependency that failed in the error so we can log it later? I guess useWithStaleFast doesn't log if it fails?
There was a problem hiding this comment.
Makes sense. At first, I thought it was too annoying to see a lot of error logs when creating a new Haskell source file, so I wanted to get rid of them completely.
Maybe we should just log it in warning level (I remember at least one other plugin does this), and take a look at why HieAst is missing for files with only a line of module header.
There was a problem hiding this comment.
Right, I realised when thinking about this that a plugin returning a ResponseError can be "normal" and not necessarily something that HLS needs to log as an error. I still think it's good to log them so you know what's happening, but maybe it doesn't need to be an error log.
I'm seeing this popup every time an empty source file is opened, like below:
HieASTsdoesn't have an AST for empty files (by empty files, I mean those haskell source files without anything other than the module header)I'm not sure if it's the problem of
GetHieAST, but let's just decrease the error level in the code range plugin, and handle the case whenCodeRangerule fails in a more tolerant way.