zesterer / ariadne

A fancy diagnostics & error reporting crate
https://crates.io/crates/ariadne
MIT License
1.61k stars 72 forks source link

Type inference fails with too few Builder method calls #22

Open troiganto opened 2 years ago

troiganto commented 2 years ago

This code compiles:

use ariadne::{Report, ReportKind, Source};
use std::ops::Range;

fn main() {
    Report::<Range<usize>>::build(ReportKind::Error, (), 10)
        .with_message("Unterminated string")
        .finish()
        .print(Source::from(""))
        .unwrap();
}

Removing the turbofish on Report like this:

use ariadne::{Report, ReportKind, Source};
use std::ops::Range;

fn main() {
    Report::build(ReportKind::Error, (), 10)
        .with_message("Unterminated string")
        .finish()
        .print(Source::from(""))
        .unwrap();
}

fails with this message:

error[E0284]: type annotations needed
 --> src/main.rs:5:5
  |
5 |     Report::build(ReportKind::Error, (), 10)
  |     ^^^^^^^^^^^^^ cannot infer type for type parameter `S`
  |
  = note: cannot satisfy `<_ as Span>::SourceId == _`
help: consider specifying the type argument in the method call
  |
6 |         .with_message::<M>("Unterminated string")
  |                      +++++

For more information about this error, try `rustc --explain E0284`.

Tested with ariadne v0.1.3 and rustc 1.58.0-nightly (ff0e14829 2021-10-31).

zesterer commented 2 years ago

This seems expected. Report::build on its own does not imply any particular type parameters. Report is generic over the span type, to a span type must be hinted at somewhere for the compiler to infer it.

troiganto commented 2 years ago

Yeah, that's what I assumed happened. I'd say it's a bit surprising that this is how Rust chose to have default type parameters, but it's not much you can change.

This is a bit of a footgun for newcomers though, as the user who initially reported this issue on Discord showcased. Is it common that someone would create a Report without calling .with_label()? This seems to be what the examples use to nail down the S parameter. If this is common enough, it might be worthwhile to at least call this issue out in the docs.

zesterer commented 2 years ago

It's quite common, yes. Without a label, there's no reference in the source code. In cases where a label is really not desired for some reason, the span type can be added to the report to allow inference.

It would be nice to resolve this, but there's not a clear way to do so. Some error messages just don't require a span (i.e: "no main function") so providing one to the report for the sake of inference would be a bit bizarre.

troiganto commented 2 years ago

Maybe it makes sense to provide a dedicated method in such cases?

impl Report<Range<usize>> {
    pub fn build_without_location(kind: ReportKind) -> ReportBuilder<Range<usize>> { ... }
}

Or maybe even impl Span for a special NoLocation type, if that's reasonable.

Another possibility would be to always start out without a location (build() only takes a ReportKind) and to provide this method:

impl ReportBuilder<NoLocation> {
    fn with_location<S, Id>(self, src_id: Id, offset: usize) -> ReportBuilder<S> {
    where
        S: Span,
        Id: Into<<S::SourceId as ToOwned>::Owned>
    { ... }
}

though in this case one would have to be careful that the generics do actually allow inferring S correctly. (I think this Id might be a little too generic, but I'm not sure …) Also, this would obviously be a breaking change.

SephVelut commented 2 years ago

The reverse order inference really does weird you out when you run into it. And you run into it immediately. Typically with combinator esque method chains, types get more specific one after the other, the further down you go into the chain. It's surprising when a call to a method, 10 lines down, effects the specificity of a generic type param 10 lines up. Worse, you might not know who is the culprit, because it is sort of globally contextual.

A -> B -> C -> D. If D doesn't type check, I should be able to blame only C. But when A breaks because D broke, then it starts to become a hunt.

zesterer commented 2 years ago

I am - very slowly - working on a refactor of ariadne, which should address this issue.