lookupMetaBlocks allows loss of data without error reporting

[andrewufrank]

Explain the problem.
The function lookupMetaBlocks is not producing an error when it fails to convert whatever found to blocks. The code in pandoc 3.6.3 is:

-- | Retrieve the metadata value for a given @key@
-- and extract blocks.
lookupMetaBlocks :: Text -> Meta -> [Block]
lookupMetaBlocks key meta =
  case lookupMeta key meta of
         Just (MetaBlocks bs)   -> bs
         Just (MetaInlines ils) -> [Plain ils]
         Just (MetaString s)    -> [Plain [Str s]]
         _                      -> []

If the value retrieved has type MetaList [..] then the value is silently lost with _ -> []

I would recommend to produce a suitable error message, eventhough such a case does not occur within the Pandoc code. -- I had constructed a value with a type of MetaList [MetaBlocks [Block]] which can be converted to [Block] but not withinlookupMetaBlocks`; an error message would have drawn my attention to the problem, finding the cause for the unexplained data loss was more difficult.

Pandoc version?
pandoc 3.6.3 on linux (debian bookworm), ghc 9.10.1

[jgm]

Well, none of these lookupMeta... functions return a type that could hold an error, so this would require changing all the types and checking for the error status in the calling sites.

If you want something fancier, it is of always possible to use lookupMeta and do the pattern matching yourself.

[andrewufrank]

I was simply thinking to replace the last line with _ -> error "The value found cannot be converted to blocks."

My issue was not with the functionality - this is easily done in a program - but with the silent discarding of data, which is hard to trace. A hard error would be helpful, when the function cannot achieve what it description says.

I cannot see a use where a value retrieved can be converted to blocks but not by this code and the programmer would rely on the silent discarding; I rather believe that the input I had is beyond the inputs the function is designed for and an error message warranted.

Thank you for your effort to maintain a wonderful tool!

[jgm]

It's considered bad practice to use error in a pure function to raise an error. (It should only be used for things that the programmer assumes should never happen.

[andrewufrank]

Completely agree! I think throwing away data silently is nothing a programmer assumes. The signature of the function allows legal values (e.g. MetaList [MetaBlocks [Block]]) which can be converted to blocks but are not and thrown away; this is a error in the program. I assume the programmer of the function did not foresee such cases. The alternative is to extend the function that it converts all convertible values to blocks; the difficulty then is how to convert MetaMap . Alternatively, on could change the signature such that only convertible types are admissable, but I do not see how....
error seems to me the best way to make the function somewhat safer but I won't insist. A warning in the description of the function would be another, not as effective, solution.

[jgm]

No -- the programmer did foresee such cases. The intent of the function is: convert this metadata field to Blocks, if it has the right shape, otherwise return an empty list. Returning an empty list for MetaList is exactly what the function was designed to do. So putting an error here would not be right. It doesn't make the function safer; indeed, it makes things less safe by raising the possibility of an unhandled exception. In pandoc's code base, we only use error in a few very rare cases, and these are all cases which "should never happen." For genuine error handling, we use throwError (in the PandocMonad context), which will produce an error that is handled appropriately, or return an Either value (in pure contexts).

If it is desirable to indicate an error when the field is not of the right shape to convert to Blocks, then the right way to handle that would be to rewrite the function (a) to return an Either value or (b) to return a value in an instance of PandocMonad. Both changes would make this function less convenient to use in the cases where it was designed to be used. That's why I'm not inclined to do it. If you want different behavior for your own purposes, it's easy enough to write your own wrapper around lookupMeta.

Adding something to the haddock comment would be fine.