feat: introduce SyntaxResult

[ematipico]

Summary

Part of #1725

Closes #1731
Closes #1740

This PR applies the following changes:

• it extends the logic of the code generation by tracking nodes that are marked as optional. The nodes/tokens that are marked as optional will emit code as Optional<Node>/Optional<SyntaxToken>. Nodes and tokens that are not marked as optional, will now generated code as SyntaxResult<Node>/ SyntaxResult<SyntaxToken>;
• created a new type called SyntaxResult;
• updated the formatter APIs to handle Result instead of Option;
• modified the manual implementation of some nodes; there, I left some TODO where we know that we will need to remove/change some logic. Some of that logic was kept there to maintain compatibility. Once we will start migrating to a better syntax, we will change (or remove) that logic too;
• updated doc tests where it was needed;
• rename FormatResult in Formatted
• create a FormatResult type that is Result<T, FormatError>
• created a FormatError which yields different types of error based on the situation
• format_root now returns a FormatResult<Formatted>

Test Plan

cargo test
cargo format
cargo lint

[jamiebuilds]

Should the formatter ever be responsible for reporting syntax errors? I assume the parser would still emit diagnostics for every syntax error even if we recover and run other tools like the formatter.

Maybe this would be the difference between logging levels though, if you passed a --log-level=verbose it would also emit these kinds of events from other tools so we can debug why the formatter didn't format what we expected.

[ematipico]

Yes it makes sense. Although executing an unwrap without a comment explaining why we don't care about the error, or a way to handle that error doesn't seem right to me, hence the TODO :)

[MichaReiser]

I think we should either return an error or return the text as is

[MichaReiser]

Nice work!

Would you mind testing if your PR affects the coverage?

Your code made it clear to me that there's an issue with the kind parameter of MissingElement. There's no obvious choice for the missing kind if the child is an union, e.g. the binary expression operator (different tokens) or if it's an expression.

I think we should either drop the param or change the error to MissingRequiredChild { parent: SyntaxNode }

[MichaReiser]

Isn't this a MissingNode error? Either the equal or value were missing. You can get the right kind by extending the match on line 17 to handle the Some(_), None and None, Some(_) cases to return a Missing error. This will be nicer once we improved the AST Facade.

[ematipico]

Unfortunately it's impossible to get the .kind() of equal and value unless we actually decide to hardcode it inside the error MissingNode.

Also, what if .value() returned a UnknownExpression? Wouldn't it count as UnknownNode? That's the part where I am struggling at the moment, understanding and handling missing nodes VS unknown nodes inside the formatter.

[MichaReiser]

Also, what if .value() returned a UnknownExpression? Wouldn't it count as UnknownNode? That's the part where I am struggling at the moment, understanding and handling missing nodes VS unknown nodes inside the formatted.

Maybe, depends on how we want to do the formatting for Unkown nodes:

• If we want to format the nodes as is, then implementing ToFormatElement for UnknownExpression is enough.
• If we want to bail out of formatting, then having an UnknownNode error is the way to go and the Unknown* implementation would just bail out with that error.

[ematipico]

Nice work!

Would you mind testing if your PR affects the coverage?

Your code made it clear to me that there's an issue with the kind parameter of MissingElement. There's no obvious choice for the missing kind if the child is an union, e.g. the binary expression operator (different tokens) or if it's an expression.

I think we should either drop the param or change the error to MissingRequiredChild { parent: SyntaxNode }

So I tried to implement MissingRequiredChild { parent: SyntaxNode } and this would required to .clone() the AstNode every time we fire an error; this is because the generic call .syntax() would return to us a reference - &SyntaxNode. Not sure if it's worth it...

Adding a { parent: &SyntaxNode} instead, would involve introduce lifetimes, inside errors. Not sure we need that.

I will remove the info about the parent for now, I think we can figure out something later.

[MichaReiser]

MissingRequiredChild

So I tried to implement MissingRequiredChild { parent: SyntaxNode } and this would required to .clone() the AstNode every time we fire an error; this is because the generic called .syntax() would return to us a reference - &SyntaxNode. Not sure if it's worth it...

Adding a { parent: &SyntaxNode} instead, would involve introducing lifetimes, inside errors. Not sure we need that.

Cloning is cheap but yeah, we don't have a use case for the param yet. Should we go with dropping the kind param for now and then add whatever we need for our tracking purposes when we need it?

[MichaReiser]

Is it worth differentiating between UnsupportedLanguage and CapabilityDisabled? Isn't the capability disabled for the case where our formatter doesn't support formatting (UnsupportedLanguage?).

[ematipico]

Yes it is worth it. Supporting a language and being able to format it should be two separate things. For example, in RomeJS we had support for parsing CSS but the format was turned off publicly. In the meantime, we could programmatically turn it on and test it, incrementally, until we were happy with the final result.

[MichaReiser]

That's fair. Is it something Formater consumers should be aware of tough? For them, it's an unsupported language.

Please go forward with what you think best.

[ematipico]

Not necessarily, we might have linting enabled while formatter still disabled (for the same language). If things changes, we are still in time for unifying everything and having the broad "unsupported" error.

[MichaReiser]

closes #1732