ruby / prism

Prism Ruby parser
https://ruby.github.io/prism/
MIT License
846 stars 143 forks source link

RFC: Add `PM_NULL_NODE` to be returned by `PM_NODE_TYPE(null)` #3189

Closed amomchilov closed 3 weeks ago

amomchilov commented 1 month ago

There's a number of times when I need to check a pm_node_t for null before discriminating on its type, like so:

something foo(pm_node_t node) {
    if (node == null) return null;

    switch (PM_NODE_TYPE(node)) {
         case PM_ALIAS_GLOBAL_VARIABLE_NODE: // ...
         // ...
    }
}

It would be nice if there was a special PM_NULL_NODE case:

/**
 * This enum represents every type of node in the Ruby syntax tree.
 */
 enum pm_node_type {
+    /** A sentinel value returned by PM_NODE_TYPE(null) **/
+    PM_NULL_NODE = 0,
+
     /** AliasGlobalVariableNode */
     PM_ALIAS_GLOBAL_VARIABLE_NODE = 1,
 }

This has 3 benefits:

  1. You can remove the explicit null check
  2. The compiler's exhaustivity checking would remind you to add case PM_NULL_NODE:
  3. It works better in a debugger, where value descriptions (like for watchpoints) will always have a valid value instead of an error on null.

Are there any downsides?

kddnewton commented 3 weeks ago

I'd rather not have this because right now all of the enums can be checked for exhaustiveness by the compiler, but if we add this then every enum everywhere has to account for a NULL node type. I would prefer explicit checks for NULL, for fear that we might accidentally introduce some other ways to get to a NULL node (since it would be the 0 value).