flyx / NimYAML

YAML implementation for Nim
https://nimyaml.org
Other
186 stars 36 forks source link

Compilation hint: 'dump' cannot raise 'YamlSerializationError' [XCannotRaiseY] #111

Closed jfilby closed 2 years ago

jfilby commented 2 years ago

I have a compilation hint when calling dump():

.nimble\pkgs\yaml-0.16.0\yaml\serialization.nim(1432, 23) Hint: 'dump' cannot raise 'YamlSerializationError' [XCannotRaiseY]

The code is on GitHub: https://github.com/jfilby/nexus/blob/main/src/nexus/cmd/service/generate/tmp_dict/tmp_dict_utils.nim Function: writeTmpDict()

flyx commented 2 years ago

The raises annotation on dump() is not strictly necessary, Nim would calculate it automatically. However, as API provider, I want the compiler to ensure that no other errors can be raised during serialization, therefore I give this explicit error as error that can possibly be raised.

Now dump() is a generic function, and which represent function(s) it calls depends on the given type. Some represent functions actually can raise YamlSerializationError, others can't. You happen to give a type which does not lead to any possible YamlSerializationErrors. Since generics are handled at compile-time, the compiler knows that no such error can ever be raised, and gives that hint.

However, I cannot remove the YamlSerializationError annotation because a call with a different type will need it. I don't want to drop the raises annotation altogether because it prevents me from accidentally introducing exceptions when maintaining this library.

To sum up: The compiler is right with this hint, but there doesn't seem to be a way to get rid of it without harming maintainability.

jfilby commented 2 years ago

OK, but perhaps it can be hidden with a -d compiler switch? When I scan for warnings it would be useful to ignore this.

flyx commented 2 years ago

Hm, I think this can be solved with

{.push hint[XCannotRaiseY]: off.}
…
{.pop.}

around the relevant declarations. Don't have the time to test this right now but I'll do that soon-ish.

flyx commented 2 years ago

So… that doesn't work. As explained in this thread, due to lazy compilation, the push/pop pragmas must be around the code that causes the dump function to compile, which is, due to lazy compilation, the calling code. I imagine you don't want to put that around every call to dump on your side.

Testing also showed that yours is not the only hint that may potentially be shown, another one is

NimYAML/yaml/serialization.nim:400:44 Hint: 'representObject' cannot raise 'ValueError'

Something should certainly be done about these. My Nim is a bit rusty (not doing anything in Nim but maintaining this lib) but I'd imagine doing something like

when defined(doNimYamlTest):
  {.push raises: […].}

# … code …

when defined(doNimYamlTest):
  {.pop.}

might work. This would remove the explicit annotations in production while still being able to check their validity during testing. The only question remaining would be how to treat this on the API docs (should they still show the annotation or not?).

This would need a larger refactoring to group all similarly annotated procs together (or put the stuff about around every single one). In case you know a more elegant solution, I'd love to hear it.

jfilby commented 2 years ago

It seems hints can be silenced with pragmas:

  {.hint[XCannotRaiseY]: off.}