[Bug] Need to revisit Move compiler error messages for V2

[brmataptos]

🐛 Bug

A few compiler error messages are confusing. Landing initial code is important, but we must remember to revisit error messages.

To reproduce

See discussions in #9002 (review).

We should enumerate them here, but that will take some time. Ideally, let's also do a diff vs move-compiler (v1) output and justify changes. But also go beyond that by revisiting the existing error messages.

Expected Behavior

Clear error messages.

System information

[brmataptos]

Looking at exp_builder.rs, I think we need to pass context into more of the calls so a correct context can be mentioned. For example, translate_exp() would be more useful if it could know " in final expression of expression sequence" + " in loop body" + " in function body"

A few of these can be improved by just fixing what's there without adding another parameter. For example, I see that a loop body type check takes a context "in expression" which is uselessly general when it could be "in loop body".

[brmataptos]

Notes on pending typing issues moved from PR 9002:

third_party/move/move-compiler-v2/tests/checking/typing/binary_add_invalid.exp:152
   = outruled candidate `+(u64, u64): u64` (expected `u64` but found `(?41, ?42)` for argument 1)
   = outruled candidate `+(u128, u128): u128` (expected `u128` but found `(?41, ?42)` for argument 1)
   = outruled candidate `+(u256, u256): u256` (expected `u256` but found `(?41, ?42)` for argument 1)
--> = outruled candidate `+(u8, u8): u8` (expected `u8` but found `(integer, integer)` for argument 1)

perhaps this one could be made more concise, perhaps something like:

(integer, integer) not compatible with argument 1 for any available + operations (u16, u32, u64, u127, u256).

----------------------------------------
third_party/move/move-compiler-v2/tests/checking/typing/constant_unsupported_exps.exp
   │         ^^^^^^^^^^

error: no function named `X::f_public` found
   ┌─ tests/checking/typing/constant_unsupported_exps.move:24:9

--

 brmataptos 2 days ago
Should this error message mention 0x42 at all? Perhaps "... at address 0x42".
--------

What happens if there is no 0x42::X::f_public() but there is an 0x41::X::f_public()?  This message would be confusing to the user unless we include the full name qualification:

"error: No function named 0x42::X::f_public found"

Is this perhaps really hard to do?  I suppose we can defer to the "error messages" PR.

----------------------------------------
third_party/move/move-compiler-v2/tests/checking/typing/declare_with_type_annot.move
    struct R {f: u64}

    fun t0() {
        let (): ();
Member
@brmataptos brmataptos 2 days ago
This is a legal let statement? It probably shouldn't be.

Member
Author
@wrwg wrwg 2 days ago
Because let (x, y) = (1, 2) is a legal one, as is let () = f(x), the above is too. Perhaps also a job for a linter.

----------------------------------------
third_party/move/move-compiler-v2/tests/checking/typing/decl_unpack_references.move
    struct R { s1: S, s2: S }

    fun t0() {
        let R { s1: S { f }, s2 }: R;
Member
@brmataptos brmataptos 2 days ago
We really should syntactically disallow such let expressions, as it just obfuscates declarations. The Move Book does present a grammar that suggests that both type-annotation and initializer are "opt", but it's so badly formatted that I'm sure no one is counting on it.

Member
Author
@wrwg wrwg 2 days ago
They are the same as in Rust, and are frequently used in Rust programs. The test cases here are definitely pathologic. In Rust, I have rarely seen nested patterns in let position, this is already much better.

Perhaps we should enforce things like readable patterns via the linter.
----------------------------------------
third_party/move/move-compiler-v2/tests/checking/typing/derefrence_invalid.exp
error: expected `bool` but found `u64` in name expression
  ┌─ tests/checking/typing/derefrence_invalid.move:6:11
  │
6 │         (*x : bool);
Member
@brmataptos brmataptos 2 days ago
This error (and the ones below) isn't quite right, as the variable x is marked, but the type u64 refers to the *x expression. x's type is &u64. Can we highlight the correct syntactic term here?

Member
Author
@wrwg wrwg 2 days ago
Good catch. This one wasn't easy but should be better now.
----------------------------------------
third_party/move/move-compiler-v2/tests/checking/typing/bind_wrong_type.exp
[[Outdated]]
@@ -1,18 +1,18 @@

Diagnostics:
error: expected `M::R` but found `M::S` in lvalue
error: expected `M::S` but found `M::R` in lvalue
Member
@brmataptos brmataptos 2 days ago
Again, this seems it would be clearer as:

error: assignment to lvalue of type M::S expected the same type but found M::R

Member
Author
@wrwg wrwg 2 days ago
Same here as above. This is a generic message generated by type unification, at the point where this happens, there is no knowledge of the assignment (and the "in lvalue" is just appeneded to this message). Its not easy to change this, so perhaps another PR.

Member
@brmataptos brmataptos 2 days ago
Ok, I filed a new issue to track this..

----
third_party/move/move-compiler-v2/tests/checking/typing/bind_wrong_type.exp
@@ -1,22 +1,22 @@

Diagnostics:
error: expected `M::R` but found `M::S` in lvalue
error: expected `M::S` but found `M::R`
  ┌─ tests/checking/typing/bind_wrong_type.move:6:13
  │
6 │         let S { g } = R {f :0};
  │             ^^^^^^^

*** Still needs to be fixed.  As is, this is a very bad message.  It might be almost ok if the RHS were highlighted, but it's not.

----------------------------------------
third_party/move/move-compiler-v2/tests/checking/typing/binary_add_invalid.exp
   = outruled candidate `+(u64, u64): u64` (expected `u64` but found `(?41, ?42)` for argument 1)
   = outruled candidate `+(u128, u128): u128` (expected `u128` but found `(?41, ?42)` for argument 1)
   = outruled candidate `+(u256, u256): u256` (expected `u256` but found `(?41, ?42)` for argument 1)
   = outruled candidate `+(u8, u8): u8` (expected `u8` but found `(integer, integer)` for argument 1)
Member
Author
@wrwg wrwg 2 days ago
Agree, but I like to do this not in this PR because the actual solution isn't that obvious and perhaps requires more discussion. We should do this definitely before users see this.

----------------------------------------
third_party/move/move-compiler-v2/tests/checking/typing/bind_unpack_references_invalid.exp
[[Outdated]]
@@ -1,24 +1,24 @@

Diagnostics:
error: expected `&u64` but found `integer` in lvalue
error: `integer` not compatible with `&u64` in lvalue
  ┌─ tests/checking/typing/bind_unpack_references_invalid.move:7:9
Member
Author
@wrwg wrwg 2 days ago
Good idea. Lets keep this for later. I think that the reality is that you rarely find complex inference examples like in those tests, but there are multiple ways how the this can be improved, yet not the highest priority in my mind. The current level of messages has worked so far for specs.

To be clear, I like the idea, but implementing it is by itself a large PR, it would need to rewrite the type unifier. The error reporting is interwoven with the way how unification works.

** This one needs more discussion.

----------------------------------------

[vineethk]

Some related issues/PRs when we tackle this: #9861, #9863, test cases in the change list of this PR: #10505.