Remove `HoverContentsEmpty` from `HoverContents`

[Hogeyama]

HoverContentsEmpty, a constructor added in this commit, does not exist in original Language Server Protocol.

interface Hover {
    contents: MarkedString | MarkedString[] | MarkupContent;
    range?: Range;
}

This results in hie sending an invalid message to the client.
Isn't it better to remove this constructor?
Semigroup would be enough if we want to combine results from multiple sources.

[alanz]

HoverContentsEmpty was added because the spec says the reply can be

result: Hover | null defined as follows:

and we did not support the null case.

This was as requested in #141

[Hogeyama]

I'm sorry for missing the issue.
The spec says Hover is nullable, but Hover.contents is not, doesn't it?
I think what we need to fix the issue is to modify the FromJSON instance like this:

instance FromJSON HoverContents where
  parseJSON v@(A.String _) = HoverContentsMS . singleton <$> parseJSON v -- PlainString
  parseJSON v@(A.Array _)  = HoverContentsMS <$> parseJSON v             -- [MarkedString]
  parseJSON v@(A.Object _) = HoverContents   <$> parseJSON v             -- MarkupContent
                         <|> HoverContentsMS . singleton <$> parseJSON v -- CodeString
  parseJSON _ = mempty

where singleton x = List [x]. (I didn't check this code actually works.)

[alanz]

Well, according to the spec, a Response message has

	/**
	 * The result of a request. This member is REQUIRED on success.
	 * This member MUST NOT exist if there was an error invoking the method.
	 */
	result?: string | number | boolean | object | null;

So we encode HoverContentsEmpty at the JSON level to be an explicit null value, for when there is no hover result.

That said, the change came about from a request for a student project, so perhaps the clients in the wild do not support this. But I am loathe to remove something that is actually specified to be like this. By my interpretation of the spec.

@bubba ?

[Hogeyama]

I think the null value is already encoded as Maybe in HoverRequest and HoverResponse:

type HoverRequest = RequestMessage ClientMethod TextDocumentPositionParams (Maybe Hover)
type HoverResponse = ResponseMessage (Maybe Hover)

In other words, what is the difference between the following result1 and result2?

result1, result2 :: Maybe Hover
result1 = Nothing
result2 = Just (Hover HoverContentsEmpty Nothing)

[alanz]

The HoverResponse is encoded for JSON using

lspOptions = defaultOptions { omitNothingFields = True, fieldLabelModifier = drop 1 }

so setting the response to Nothing omits the field completely.

This is different from explicitly encoding it as contents: null, which the spec allows.

[Hogeyama]

No, it isn't. Because ResponseMessage is defined as follows,

data ResponseMessage a =
  ResponseMessage
    { _jsonrpc :: Text
    , _id      :: LspIdRsp
    , _result  :: Maybe a
    , _error   :: Maybe ResponseError
    } deriving (Read,Show,Eq)
deriveJSON lspOptions ''ResponseMessage

HoverResponse has nested Maybe:

type HoverResponse
  = ResponseMessage
    { _jsonrpc :: Text
    , _id      :: LspIdRsp
    , _result  :: Maybe (Maybe Hover)
    , _error   :: Maybe ResponseError
    }

So, Core.makeResponseMessage req result1 is a record like

ResponseMessage
  { _jsonrpc = "2.0"
  , _id      = 1
  , _result  = Just Nothing
  , _error   = Nothing
  }

and encoded into the following json

{"jsonrpc":"2.0","id":1,"result":null}

Indeed, hie has sent above message until this commit.

By the way, contents: null is a typo of result: null?

[alanz]

Yes, it was a typo, I meant result.

I will sort this out soon, I am in the midst of another PR, don't want to do two things in one.

[lukel97]

@alanz @Hogeyama makes sense to me to drop that constructor. Nulls are indeed the billion dollar mistake