[Bug][move-compiler-v2] move-model AST's `ExpData::Invalid` is useless as documented

[brmataptos]

🐛 Bug

See move-model/src/ast.rs:

    /// Represents an invalid expression. This is used as a stub for algorithms which
    /// generate expressions but can fail with multiple errors, like a translator from
    /// some other source into expressions. Consumers of expressions should assume this
    /// variant is not present and can panic when seeing it.
    Invalid(NodeId),

This comment indicates that it is unsafe to ever generate an Invalid expression, as subsequent code may just panic.

Instead, we need a policy that all code encountering such a construct silently tolerates it while returning/exiting, so that it can be safely use as a placeholder for an ExpData value after an error is generated.

[brmataptos]

@wrwg

[wrwg]

Invalid is used and needed.

For example, the type checker produces an expression AST and when it encounters an error, it inserts Invalid. After type checking, processing does not continue, so no one is crashing on it.

[brmataptos]

The comment contradicts what you have stated and needs to be corrected. exp_builder should be able to introduce Invalid along the way as a placeholder after an error while processing other code, and not be panicking before it's finished. In, fact fn new_error_exp is called all over exp_builder, creating lots of Invalid expressions.

But as documented in that comment, the enum variant ExpData::Invalid is useless.

Did you even look at the comment before closing this bug?