Support multi-span errors and warnings

[sezna]

Often, you want to display help text or render multiple spans in an error.

[emilyaherbert]

Can you explain a little more what you mean by this?

[sezna]

Yeah, so our error renderer in forc currently can only render one span per error/warning. In Rust, sometimes there are errors that show multiple spans, like, "this value was moved here but used again here ". This issue involves:

1. Identifying which, if any, issues would benefit from multiple spans (using Rust as inspiration).
2. Implementing a way for errors/warnings to highlight multiple spans when they're rendered.

[emilyaherbert]

A good one to start might be "not declared as mutable... but used as mutable here ^". What do you think?

[sezna]

related: #346

[vaivaswatha]

A good one to start might be "not declared as mutable... but used as mutable here ^". What do you think?

Another error that might benefit from this: #751. "Function ^ is being redefined. It was already previously defined ^"

[anton-trunov]

One more issue that would benefit from this: #3532. The CEI pattern analysis would highlight just two lines: the one with the external call and the one with storage modification or some other effect.

[mohammadfawaz]

I recall that we tried to do this but then reverted the change for some reason? Is this too difficult to implement? I do find myself needing this often. cc @sezna. Also, if it's too complex, we should probably remove the good first issue tag 😄

[sezna]

There was a bug previously where the compiler would panic if the two errors were from different files, which was usually the case, so we reverted the old implementation.

[ironcev]

Here is the progress so far and two open questions to agree on. Please provide your feedback on the questions.

Before and after :-)

Open Questions

Structure of compile messages

What do we want to support in our compile messages and how much freedom, or restriction do we want to offer. For the sake of consistency, I am for a limited, clearly defined number of elements that we can embed in a compile message. This way we can impose consistency across all our messages. So far I support these elements, and enforce them via the compile message (CM) API:

• CM can have a label.
• CM must have exactly one (looking for a better term) message that can be an error or a warning. This is what we have as a single error/warning right now.
• CM can provide additional info. These are references to other places in code related to the core message. They can span over different files.
• CM can provide help. Help is a plain text. We can provide arbitrary many help strings.
• Info refer to places in files, whereas the message itself can (and mostly will) but does not have to (we have such situation now).

You can spot all of these elements on the above image.

What do you think of this approach? Do you see any other elements that we could need?

Declarative API

At the moments messages are declared imperatively, pretty much like the way we do warnings right now. This means a lot of clutter and boilerplate. E,g,:

VariableShadowsConstant { name , constant_span} => CompileMessage {
    title: Some("Constants cannot be shadowed".to_string()),
    message: InSourceMessage::error(
          source_engine,
          self.span().clone(),
          format!("Variable \"{name}\" shadows constant with the same name.")
    ),
    in_source_info: vec![InSourceMessage::info(
         source_engine,
         constant_span.clone(),
         format!("Constant \"{name}\" is declared here.")
    )],
    help: vec![
        "Unlike variables, constants cannot be shadowed by other constants or variables, nor they can shadow variables.".to_string()
    ],
    ..Default::default()
},

The proposal is to define our own macros for simplifying the message definition. Something along these lines:

#[label("Constants cannot be shadowed")]
#[error(name.span(), "Variable \"{name}\" shadows constant with the same name.")]
#[info(constant_span, "Constant \"{name}\" is declared here.")]
#[help("Unlike variables, constants cannot be shadowed by other constants or variables, nor they can shadow variables.")]
VariableShadowsConstant { name: Ident, constant_span: Span },

[IGI-111]

I really like this approach. In general having a formal set of items we can use to explain a problem to the user is the right way to go.

I guess if I had to criticize this I would say that it's not very clear at a glance to the compiler dev what a "label" or an "info" is and how different they will look to the user. Ideally I'd want it to be easy to design a decent error without having to look up the spec.
Perhaps we can get close with better naming or something?

Another thing is that this:

#[label("Constants cannot be shadowed")]
#[error(name.span(), "Variable \"{name}\" shadows constant with the same name.")]
#[info(constant_span, "Constant \"{name}\" is declared here.")]
#[help("Unlike variables, constants cannot be shadowed by other constants or variables, nor they can shadow variables.")]
VariableShadowsConstant { name: Ident, constant_span: Span },

is probably expressed as a single proc-macro:

#[error(
    name = name.span(),
    msg = "Variable \"{name}\" shadows constant with the same name.",
    label = "Constants cannot be shadowed",
    info = (constant_span, "Constant \"{name}\" is declared here."),
    help = "Unlike variables, constants cannot be shadowed by other constants or variables, nor they can shadow variables.",
)]
VariableShadowsConstant { name: Ident, constant_span: Span },

One may also want to have more than a single "info", though I would expect the cases where this is necessary are rare.

[ironcev]

Completely agree that the terminology is very confusing. I wasn't happy with it either, especially when modeling the corresponding structs in code (CompilerMessage having field message of type InSourceMessage 😱 😱). But that's the first pass and you know how it is - naming things, the most difficult problem in computer science :-)

After pondering about it (a lot) and also partially inspired with the The Anatomy of Error Messages in Rust talk (@IGI-111 thanks for the suggestion) I propose the following:

The whole thing is called Diagnostic and has:

• level: Error or Warning.
• reason: Why is this an error/warning? Because... Constants cannot be shadowed. This identifies the whole family of concrete errors/warnings.
• issue: The issue that triggered the diagnostic. This is a concrete error: Variable "y" shadows constant with the same name.
• hints: Additional information about the issue. Related lines of code, symbols, etc.
• help: More insights about the issue. Suggestions how to solve it, etc.

To this, we should also add id (or code) as suggested in #3512. It's the unique identifier of the reason actually. I wouldn't call it error code simply because it can be a warning code as well.

The mandatory fields are:

• level
• id (once we introduce it)
• reason (so far not mandatory for backward compatibility)
• issue

Everything else is optional. We can have arbitrary number of hints spanning different files and arbitrary number of help lines.

I love the proposal of having a single proc-macro! And it doesn't mean that we cannot have several hints or help lines that way. E.g.:

#[error(
    id = "E01007",
    reason = "Constants cannot be shadowed",
    issue = (name.span(), "Constant \"{name}\" shadows imported constant with the same name."),
    hint = (constant_import_span, "Constant \"{name}\" is imported here."),
    hint = (constant_decl_span, "Constant \"{name}\" is declared here."),
    help = "Unlike variables, constants cannot be shadowed by other constants or variables, nor they can shadow variables.",
    help = "Consider renaming the constant or use an alias for the imported constant.",
)]
ConstantShadowsImportedConstant { name: Ident, constant_import_span: Span, constant_decl_span: Span },

[ironcev]

For the record, a very inspiring paper titled Concepts Error Messages for Humans. It emphasize the importance of good (great!) error messages and provides concrete tips how to formulate them. For the purpose of this discussion, everything proposed in the paper can be achieved with the intentionally restricted number of supported elements in Diagnostics, listed above.

The article also mentions the miette crate. Miette is an all-in-one library for defining, collecting, and rendering errors, compatible with anyhow and thiserror. Which interestingly have the #[diagnostic] macro similar to what we discussed:

#[diagnostic(
    code(oops::my::bad),
    url(docsrs),
    help("try doing it better next time?")
)]

(Yes, at glance, it looks like what we did developing our own infrastructure and combining thiserror and annotate-snippets, Miette does out of the box. But more than a glance is needed to support this claim.)

[IGI-111]

The new version looks better, I'm really looking forward to having error ids, if only because once we do we'll be able to provide #[allow()] style annotations.

I do think we made the right choice rolling our own error rendering if only because it gives us flexibility in how we want to tackle the problem, but miette is indeed pretty good and we could certainly take cues out of how it's designed.