More accurately provide spans to errors in the GodotClass macro

[lilizoey]

This makes rust actually show where the errors are when the proc macro errors. Some examples:

and

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-920

[Bromeon]

Very nice, this is a huge quality-of-life improvement!

This reminds me of #641, which could in the future probably be reviewed in a similar way?

[Bromeon]

I usually tried to keep venial types qualified, since they often have very generic names (like here 🙂).

Suggested change

	pub errors: Vec<Error>,
	pub errors: Vec<venial::Error>,

[Bromeon]

Maybe add a comment that we still need to generate code in case of bad base syntax, so that spans are retained/propagated to the compiler error.

[lilizoey]

it's more about avoiding confusing/misleading errors. for instance if you make the base field an OnReady then you'll get errors pointing at the GodotClass macro that OnReady is missing a to_gd() function. but on top of that you'll also get the error appropriately pointing at the base field, telling you you can't use OnReady for the base field. this code ensures that only the latter error triggers, as that's the more important and useful error.

[lilizoey]

though this does actually cause an issue here:

where there is only an error at the #[var], and no error corresponding to the type being wrong like here:

i wonder if there's a good way to fix both

[lilizoey]

i've mostly figured out a nicer way to fix this actually, but one problem i keep having is that i can't find a good way to get rust to report a nice error when the type is wrong. or rather to stop reporting a bad error. because the init auto generated code creates code like this (simplified some long paths for readability):

impl GodotDefault for Foo {
    fn __godot_user_init(base: Base<Self::Base>) -> Self {
        Self {
            base: base,
        }
    }
}

and if the user declares the base field to have the type say Option<Gd<Node2D>>, then rust will report an error that an Option<Gd<Node2D>> was expected (since that's the type of the base field) but got a Base<Node2D> (the type of the base argument). which is kinda backwards, we want rust to report that a Base<Node2D> was expected but got an Option<Gd<Node2D>>. we can relatively easily add an error message of that kind, but the original error remains anyway.

[lilizoey]

actually i think there's a way to do it, it involves making a custom trait like this:

#[diagnostic::on_unimplemented(
    message = "expected base `{Self}` got `{A}`",
    label = "expected base `{Self}` got `{A}`"
)]
#[doc(hidden)]
pub trait IsBase<T: GodotClass, A> {
    fn conv(b: Base<T>) -> A;
}
impl<T: GodotClass> IsBase<T, Base<T>> for Base<T> {
    fn conv(b: Base<T>) -> Base<T> {
        b
    }
}

which is used like this in the codegen:

impl GodotDefault for Foo {
    fn __godot_user_init(
        base: Base<Node2D>,
    ) -> Self {
        Self {
            base: <Base<Node2D> as IsBase<Node2D, Option<Gd<Node2D>>>>::conv(base),
        }
    }
}

this gives this error (it's pointing at the wrong place atm but i think that's fixable):

error[E0277]: expected base `Base<godot::prelude::Node2D>` got `Option<godot::prelude::Gd<godot::prelude::Node2D>>`
 --> src/creature.rs:7:10
  |
7 | #[derive(GodotClass)]
  |          ^^^^^^^^^^ expected base `Base<godot::prelude::Node2D>` got `Option<godot::prelude::Gd<godot::prelude::Node2D>>`
  |
  = help: the trait `IsBase<godot::prelude::Node2D, Option<godot::prelude::Gd<godot::prelude::Node2D>>>` is not implemented for `Base<godot::prelude::Node2D>`
  = help: the trait `IsBase<godot::prelude::Node2D, Base<godot::prelude::Node2D>>` is implemented for `Base<godot::prelude::Node2D>`
  = help: for that trait implementation, expected `Base<godot::prelude::Node2D>`, found `Option<godot::prelude::Gd<godot::prelude::Node2D>>`

[Bromeon]

Hm, nice idea! But I'm not really sure that the error message is very understandable. Is the benefit mostly pointing to the type instead of one of the proc-macro attributes?

We may also consider that:

• using #[hint(base)] is rare
• accidentally typing base but not Base<T> is also not a common error
• this extra trait may add indirections that have negative effects in other places (including error messages)

So maybe this PR could focus on the more general problem for now (also to not make the diff too complex)?

[lilizoey]

Hm, nice idea! But I'm not really sure that the error message is very understandable. Is the benefit mostly pointing to the type instead of one of the proc-macro attributes?

It's to have an error saying that a Base<_> was expected but an Option<Gd<T>> was given, instead of an error saying Option<Gd<_>> was expected but a Base<_> was given

We may also consider that:

* using `#[hint(base)]` is rare

* accidentally typing `base` but not `Base<T>` is also not a common error

* this extra trait may add indirections that have negative effects in other places (including error messages)

So maybe this PR could focus on the more general problem for now (also to not make the diff too complex)?

sure

[Bromeon]

I read the new error message again. The main points are quite helpful, but this one is confusing:

help: the trait IsBase<godot::prelude::Node2D, Option<godot::prelude::Gd<godot::prelude::Node2D>>> is not implemented for Base<godot::prelude::Node2D>

People would then look for an IsBase trait (it becomes a leaky abstraction). Unfortunately, it seems like there's only so much we can do regarding good error messages. In this case, I hope the error occurs rare enough that we don't need to add machinery to make it slightly better.

So you think you can revert the IsBase addition?

[lilizoey]

Very nice, this is a huge quality-of-life improvement!

This reminds me of #641, which could in the future probably be reviewed in a similar way?

Probably? We dont really provide accurate spans everywhere atm, so doing that properly would likely help a lot of issues like this.

[Bromeon]

Rebased to integrate clippy fix from #929.

[Bromeon]

Thanks a lot!