Fix missing/invalid class/module names

[thomasmarshall]

This ensures running Sorbet with the Prism parser mode does not crash on missing or invalid class/model constant paths. See the regression tests for examples that were crashing without the fix.

The fix is to check constant_path exists and is one of the expected node types (ConstantReadNode or ConstantPathNode). If it doesn't exist or is an unexpected node type, we construct a "constant missing" constant to use in its place.

For example:

module
end

In this case, constant_path is not set and the body is empty.

module
  def foo
  end
end

Here constant_path is a DefNode and the body is empty.

module
  def foo
  end

  def bar
  end
end

Here constant_path is a DefNode and the body contains the second DefNode inside a StatementsNode.

The behavior is different to the original parser, which throws away much more of the class/module. Here we're able to keep most of it and only throw away whatever node was incorrectly in the constant_path field.

See here for more information. I closed that issue because I thought it was no longer crashing, but upon testing I can see that it still is.

[amomchilov]

It differs from the legacy parser, but I think we can make this even by better by not discarding def foo, but instead making it the start of the body

[thomasmarshall]

Yeah, possible, but I'm not sure it's always obvious that this node should be part of the body. These are the examples from Prism:

class (return)::A; end
       ^~~~~~ unexpected void value expression

class 0.X end
      ^~~ unexpected constant path after `class`; class/module name must be CONSTANT

module Parent module end
              ^~~~~~ unexpected constant path after `module`; class/module name must be CONSTANT
                     ^~~ unexpected 'end', assuming it is closing the parent module definition

I think here it doesn't necessarily make sense to take that unexpected node and shove it into the body. What do you think?

[amomchilov]

Hmm idk, it seems like there's time when it's the obvious thing to do, and these cases where it's clearly wrong. Not sure which is more prevalent.

One of the potential benefits is minimizing churn in the tree, which then means Sorbet's incremental type checking has to do less. E.g. if you remove the class name, the first member will disappear temporarily until you write the new name in. In the mean time, you could cause a lot of type checking errors. Though now that I think about it, you probably had those errors anyway, from the class being temporarily undefined.

IDK, we can revisit this later as a potential improvement. The original pipeline dropped more than Prism, so we're already "ahead" anyway

[amomchilov]

I think this is only repeated twice, but could you still extract a shared function anyway? I imagine this will always want to be kept the same between the class and module cases

[amomchilov]

Hmm idk, it seems like there's time when it's the obvious thing to do, and these cases where it's clearly wrong. Not sure which is more prevalent.

One of the potential benefits is minimizing churn in the tree, which then means Sorbet's incremental type checking has to do less. E.g. if you remove the class name, the first member will disappear temporarily until you write the new name in. In the mean time, you could cause a lot of type checking errors. Though now that I think about it, you probably had those errors anyway, from the class being temporarily undefined.

IDK, we can revisit this later as a potential improvement. The original pipeline dropped more than Prism, so we're already "ahead" anyway