[compiler v1] Better compiler error messages (#9222) #10505

fEst1ck · 2023-10-13T19:26:33Z

Description

This improves the compiler error messages when type inference fails by

removing duplicated error messages for the same uninferred type variable (uninferred_type_unpack_assign.exp).
telling why we cannot infer; e.g., which generic needs to be provided.

Test Plan

Existing tests of compiler v1.

brmataptos

Changing type_msg_ to a more meaningful name and having it return bool to indicate that the type could not be inferred and the caller should issue a warning would be more useful than creating a lot of warning messages that mostly won't be used.

third_party/move/move-compiler/src/typing/expand.rs

brmataptos · 2023-10-13T22:46:02Z

third_party/move/move-compiler/src/typing/expand.rs

+   type_msg_(context, ty, &format!("Cannot infer the type parameter {param_name} for generic function {m}::{n}. Try providing a type parameter."));
+}
+
+pub fn type_msg_(context: &mut Context, ty: &mut Type, msg_uninferred: &str) {


This name isn't not self-explanatory. Add a comment, perhaps something like:

/// Try to expand the type of ty, warning with msg_uninferred if type cannot be inferred.

HOWEVER, I think it would be better to change this method to try_to_infer_type(context, ty): bool which returns true on success, and have the message be created only if the it returns failure. This would avoid creating a lot of unneeded warning messages, just in case they might be needed.

I was halfway transitting, and realized this: there cannot be a try_to_infer_type(context, ty) -> bool that lets the caller choose what to do when type inference fails. Consider when we are expanding S<X, Y>, and we cannot infer X. The caller only gets a false, and doesn't know which parameter cannot be inferred, so no way to provide proper diagnostics.

Admittedly, type_msg_ is a bad name, but I derived that from the original type_, which uses a general warning for uninferred types.

Hence I will maintain the current design, and add comments for type_msg_.

vineethk · 2023-10-17T18:33:44Z

third_party/move/move-compiler/src/typing/expand.rs

-pub fn type_(context: &mut Context, ty: &mut Type) {
+// type_ for expanding the `i`-th type param of struct `ty_name`
+fn type_struct_ty_param(context: &mut Context, ty: &mut Type, i: usize, ty_name: &TypeName_) {
+    match ty_name {


Consider using if let destructuring here, which is nice to use when you have a match with just 2 branches, one of them being _.

third_party/move/move-compiler/src/typing/expand.rs

brmataptos · 2023-10-19T17:40:07Z

third_party/move/move-compiler/src/typing/expand.rs

+                    Type_::Var(i) => {
+                        let last_tvar = forward_tvar(subst, i);
+                        match subst.get(last_tvar) {
+                            Some(sp!(_, Type_::Var(_))) => unreachable!(),


If this is truly unreachable, why do you need to test for it? The Some(inner) clause below encapsulates this case.

This is the decision made by the original author of unfold_type. I think this is intended as a debug_assert.

Use panic! with message as on line 114 in this code.

Otherwise its good practice to have this case explicit

If this is truly unreachable, why do you need to test for it? The Some(inner) clause below encapsulates this case.

It should be truly unreachable, and this is like a debug assert.

brmataptos · 2023-10-19T17:51:12Z

third_party/move/move-compiler/src/typing/expand.rs

+}
+
+// Try to expand the type of ty, warning with msg_uninferred if type cannot be inferred.
+fn type_msg_(context: &mut Context, ty: &mut Type, msg_uninferred: &str) {


Better to use a lambda parameter for msg_uninferred so you can defer string construction unless it's needed. Probably need a dyn Fun to avoid generic expansion, though.

There doesn't seem to be any use case of type_msg_ where we know in advance that the expansion will always succeed. And since we are transitioning to compiler v2, I wonder if we need this extra power.

wrwg · 2023-10-19T22:43:56Z

third_party/move/move-compiler/src/typing/expand.rs

    use Type_::*;
    match &mut ty.value {
        Anything | UnresolvedError | Param(_) | Unit => (),
        Ref(_, b) => type_(context, b),
        Var(tvar) => {
            let ty_tvar = sp(ty.loc, Var(*tvar));
-            let replacement = core::unfold_type(&context.subst, ty_tvar);
+            fn unfold_type_mut(subst: &mut Subst, sp!(loc, t_): Type) -> Type {


Please do not nest function definitions deeply inside other definitions, particular if its large like this. Pull to the outer scope. Also why is there mut in the function name?

wrwg · 2023-10-19T22:49:29Z

third_party/move/move-compiler/src/typing/expand.rs

+                    Type_::Var(i) => {
+                        let last_tvar = forward_tvar(subst, i);
+                        match subst.get(last_tvar) {
+                            Some(sp!(_, Type_::Var(_))) => unreachable!(),


Use panic! with message as on line 114 in this code.

Otherwise its good practice to have this case explicit

wrwg · 2023-10-19T22:51:51Z

third_party/move/move-compiler/src/typing/expand.rs

+}
+
+// Try to expand the type of ty, warning with msg_uninferred if type cannot be inferred.
+fn type_msg_(context: &mut Context, ty: &mut Type, msg_uninferred: &str) {


Better name then type_msg_ as type_with_context_msg? Also the convention in this code to use a trailing _ in a name is when you try to avoid a keyword, or you are processing the non-spanned version of a type, which is not the case here, so no _ needed at the end of the name.

wrwg · 2023-10-19T22:59:22Z

third_party/move/move-compiler/src/typing/expand.rs

    use Type_::*;
    match &mut ty.value {
        Anything | UnresolvedError | Param(_) | Unit => (),
        Ref(_, b) => type_(context, b),
        Var(tvar) => {
            let ty_tvar = sp(ty.loc, Var(*tvar));
-            let replacement = core::unfold_type(&context.subst, ty_tvar);
+            fn unfold_type_mut(subst: &mut Subst, sp!(loc, t_): Type) -> Type {


Why is core::unfold_type not doing what you are trying to achieve here? It should unfold the type by its final replacement, does it not?

codecov · 2023-11-15T05:09:56Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (f8d673f) 71.6% compared to head (5844979) 68.9%.
Report is 5 commits behind head on main.

Files	Patch %	Lines
...hird_party/move/move-compiler/src/typing/expand.rs	96.0%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #10505       +/-   ##
===========================================
- Coverage    71.6%    68.9%     -2.8%     
===========================================
  Files        2031      753     -1278     
  Lines      384787   172219   -212568     
===========================================
- Hits       275757   118762   -156995     
+ Misses     109030    53457    -55573

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fEst1ck · 2023-10-19T18:16:50Z

third_party/move/move-compiler/src/typing/expand.rs

+                    Type_::Var(i) => {
+                        let last_tvar = forward_tvar(subst, i);
+                        match subst.get(last_tvar) {
+                            Some(sp!(_, Type_::Var(_))) => unreachable!(),


This is the decision made by the original author of unfold_type. I think this is intended as a debug_assert.

fEst1ck · 2023-10-19T18:25:44Z

third_party/move/move-compiler/src/typing/expand.rs

+}
+
+// Try to expand the type of ty, warning with msg_uninferred if type cannot be inferred.
+fn type_msg_(context: &mut Context, ty: &mut Type, msg_uninferred: &str) {


There doesn't seem to be any use case of type_msg_ where we know in advance that the expansion will always succeed. And since we are transitioning to compiler v2, I wonder if we need this extra power.

fEst1ck · 2023-11-15T22:32:41Z

third_party/move/move-compiler/src/typing/expand.rs

            let replacement = match replacement {
                sp!(_, Var(_)) => panic!("ICE unfold_type_base failed to expand"),
                sp!(loc, Anything) => {
-                    let msg = "Could not infer this type. Try adding an annotation";
                    context


This is to avoid duplicate error messages for uninferred type variables. In the first time they are resolved to Err(_), and Anything for all following queries.

Please add a comment with this text.

fEst1ck · 2023-11-15T22:44:05Z

third_party/move/move-compiler/src/typing/expand.rs

-pub fn type_(context: &mut Context, ty: &mut Type) {
+// type_ for expanding the `i`-th type param of struct `ty_name`
+fn type_struct_ty_param(context: &mut Context, ty: &mut Type, i: usize, ty_name: &TypeName_) {
+    match ty_name {


fEst1ck · 2023-11-15T22:45:45Z

third_party/move/move-compiler/src/typing/expand.rs

+                    Type_::Var(i) => {
+                        let last_tvar = forward_tvar(subst, i);
+                        match subst.get(last_tvar) {
+                            Some(sp!(_, Type_::Var(_))) => unreachable!(),


If this is truly unreachable, why do you need to test for it? The Some(inner) clause below encapsulates this case.

It should be truly unreachable, and this is like a debug assert.

fEst1ck · 2023-11-15T22:45:54Z

third_party/move/move-compiler/src/typing/expand.rs

+                    Type_::Var(i) => {
+                        let last_tvar = forward_tvar(subst, i);
+                        match subst.get(last_tvar) {
+                            Some(sp!(_, Type_::Var(_))) => unreachable!(),


fEst1ck · 2023-11-15T22:46:11Z

third_party/move/move-compiler/src/typing/expand.rs

+}
+
+// Try to expand the type of ty, warning with msg_uninferred if type cannot be inferred.
+fn type_msg_(context: &mut Context, ty: &mut Type, msg_uninferred: &str) {


fEst1ck · 2023-11-15T22:46:28Z

third_party/move/move-compiler/src/typing/expand.rs

    use Type_::*;
    match &mut ty.value {
        Anything | UnresolvedError | Param(_) | Unit => (),
        Ref(_, b) => type_(context, b),
        Var(tvar) => {
            let ty_tvar = sp(ty.loc, Var(*tvar));
-            let replacement = core::unfold_type(&context.subst, ty_tvar);
+            fn unfold_type_mut(subst: &mut Subst, sp!(loc, t_): Type) -> Type {


fEst1ck · 2023-11-15T22:48:58Z

PTLA

brmataptos

Please see 2 suggested comments to add.

brmataptos · 2023-11-15T23:46:37Z

third_party/move/move-compiler/src/typing/expand.rs

                },
                _ => panic!("ICE impossible. tapply switched to nontapply"),
            }
        },
    }
 }

+pub fn type_(context: &mut Context, ty: &mut Type) {


While you're modifying this, can you add a comment to say what this function does? Perhaps

// Try to expand the type of ty, warning with a default message if type cannot be inferred.

This whole file is woefully undocumented. Thanks for figuring it out for us.

brmataptos · 2023-11-15T23:50:59Z

third_party/move/move-compiler/src/typing/expand.rs

            let replacement = match replacement {
                sp!(_, Var(_)) => panic!("ICE unfold_type_base failed to expand"),
                sp!(loc, Anything) => {
-                    let msg = "Could not infer this type. Try adding an annotation";
                    context


Please add a comment with this text.

fEst1ck · 2023-11-16T17:49:34Z

PTLA @vineethk @wrwg

vineethk

lgtm.

For error messages on uninferred type vars, show the message only once, instead for each occurence of the same type var. For instance, if we have a generic struct `S<T>`. Compare the current ``` error[E04010]: cannot infer type ┌─ tests/move_check/typing/my_test0.move:5:9 │ 5 │ S{} = S{}; │ ^^^ Could not infer this type. Try adding an annotation error[E04010]: cannot infer type ┌─ tests/move_check/typing/my_test0.move:5:15 │ 5 │ S{} = S{}; │ ^^^ Could not infer this type. Try adding an annotation ``` with the new ``` ---- move_check_testsuite::typing/my_test0.move ---- Expected success. Unexpected diagnostics: error[E04010]: cannot infer type ┌─ tests/move_check/typing/my_test0.move:5:9 │ 5 │ S{} = S{}; │ ^^^ Could not infer this type. Try adding an annotation ```

github-actions · 2023-11-17T16:34:13Z

✅ Forge suite `realistic_env_max_load` success on `58449790b296a56ca0dc43f05c53bac66fc4ea3f`

two traffics test: inner traffic : committed: 7496 txn/s, latency: 5230 ms, (p50: 5100 ms, p90: 6300 ms, p99: 10800 ms), latency samples: 3238300
two traffics test : committed: 100 txn/s, latency: 2537 ms, (p50: 2300 ms, p90: 2900 ms, p99: 5800 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.206, avg: 0.197", "QsPosToProposal: max: 0.195, avg: 0.175", "ConsensusProposalToOrdered: max: 0.706, avg: 0.663", "ConsensusOrderedToCommit: max: 0.602, avg: 0.582", "ConsensusProposalToCommit: max: 1.286, avg: 1.244"]
Max round gap was 1 [limit 4] at version 1624487. Max no progress secs was 4.226278 [limit 10] at version 1624487.
Test Ok

github-actions · 2023-11-17T16:35:21Z

✅ Forge suite `compat` success on `aptos-node-v1.7.3` ==> `58449790b296a56ca0dc43f05c53bac66fc4ea3f`

Compatibility test results for aptos-node-v1.7.3 ==> 58449790b296a56ca0dc43f05c53bac66fc4ea3f (PR)
1. Check liveness of validators at old version: aptos-node-v1.7.3
compatibility::simple-validator-upgrade::liveness-check : committed: 3551 txn/s, latency: 6511 ms, (p50: 6300 ms, p90: 10100 ms, p99: 12600 ms), latency samples: 184700
2. Upgrading first Validator to new version: 58449790b296a56ca0dc43f05c53bac66fc4ea3f
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1852 txn/s, latency: 15638 ms, (p50: 18700 ms, p90: 21900 ms, p99: 22300 ms), latency samples: 92600
3. Upgrading rest of first batch to new version: 58449790b296a56ca0dc43f05c53bac66fc4ea3f
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1467 txn/s, latency: 18690 ms, (p50: 18900 ms, p90: 23900 ms, p99: 25200 ms), latency samples: 73380
4. upgrading second batch to new version: 58449790b296a56ca0dc43f05c53bac66fc4ea3f
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3674 txn/s, latency: 8726 ms, (p50: 9600 ms, p90: 12600 ms, p99: 12700 ms), latency samples: 143300
5. check swarm health
Compatibility test for aptos-node-v1.7.3 ==> 58449790b296a56ca0dc43f05c53bac66fc4ea3f passed
Test Ok

github-actions · 2023-11-17T16:47:45Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.7.3` ==> `58449790b296a56ca0dc43f05c53bac66fc4ea3f`

Compatibility test results for aptos-node-v1.7.3 ==> 58449790b296a56ca0dc43f05c53bac66fc4ea3f (PR)
Upgrade the nodes to version: 58449790b296a56ca0dc43f05c53bac66fc4ea3f
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 7004 txn/s, latency: 4810 ms, (p50: 4800 ms, p90: 7800 ms, p99: 8400 ms), latency samples: 245160
5. check swarm health
Compatibility test for aptos-node-v1.7.3 ==> 58449790b296a56ca0dc43f05c53bac66fc4ea3f passed
Test Ok

Improve the compiler error messages when type inference fails by - removing duplicated error messages for the same uninferred type variable (see `uninferred_type_unpack_assign.exp`). - telling why we cannot infer; e.g., which generic needs to be provided.

fEst1ck linked an issue Oct 13, 2023 that may be closed by this pull request

[Bug] Misleading compiler error when passing the wrong type to a function with generics #9222

Closed

fEst1ck requested review from wrwg, vineethk and brmataptos October 13, 2023 19:55

brmataptos reviewed Oct 13, 2023

View reviewed changes

vineethk reviewed Oct 17, 2023

View reviewed changes

vineethk mentioned this pull request Oct 17, 2023

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

Closed

fEst1ck force-pushed the better-compiler-err-msg branch from 6d2f112 to 1195c84 Compare October 18, 2023 02:48

fEst1ck requested review from vineethk and brmataptos October 18, 2023 03:03

brmataptos reviewed Oct 19, 2023

View reviewed changes

wrwg reviewed Oct 19, 2023

View reviewed changes

fEst1ck force-pushed the better-compiler-err-msg branch from 28ee3bb to e926c16 Compare November 15, 2023 04:52

fEst1ck force-pushed the better-compiler-err-msg branch from e926c16 to ef93ebc Compare November 15, 2023 22:38

fEst1ck commented Nov 15, 2023

View reviewed changes

fEst1ck requested review from wrwg and brmataptos November 15, 2023 22:48

fEst1ck force-pushed the better-compiler-err-msg branch from 774b23e to 950fa82 Compare November 15, 2023 22:50

brmataptos approved these changes Nov 15, 2023

View reviewed changes

fEst1ck force-pushed the better-compiler-err-msg branch from 950fa82 to 4b6645b Compare November 16, 2023 01:32

vineethk approved these changes Nov 17, 2023

View reviewed changes

Zekun Wang added 6 commits November 17, 2023 10:23

[compiler] improve error message struct

dcea4f4

update tests for new err msg

9c87410

improve built-in function err msg on uninferred generic

8cd3039

compiler err msg on uninferred unpack generics

7611c44

compiler err msg uninferred function generics

638ce46

Zekun Wang added 10 commits November 17, 2023 10:23

update test cases

855943e

modify err msg

69b15c4

update test cases

6a37af1

format code

7ac1274

add backquote around code in warnings

20fcb83

add comments

82cad8a

refactor

9a97140

refactor

e136f9c

typo

f031905

add comments

5844979

fEst1ck force-pushed the better-compiler-err-msg branch from 4b6645b to 5844979 Compare November 17, 2023 15:23

fEst1ck enabled auto-merge (squash) November 17, 2023 16:09

This comment has been minimized.

Sign in to view

fEst1ck merged commit bf9db4b into aptos-labs:main Nov 17, 2023
90 checks passed

[compiler v1] Better compiler error messages (#9222) #10505

[compiler v1] Better compiler error messages (#9222) #10505

Conversation

fEst1ck commented Oct 13, 2023

Description

Test Plan

brmataptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fEst1ck commented Nov 15, 2023

brmataptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fEst1ck commented Nov 16, 2023

vineethk left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 17, 2023

✅ Forge suite realistic_env_max_load success on 58449790b296a56ca0dc43f05c53bac66fc4ea3f

github-actions bot commented Nov 17, 2023

✅ Forge suite compat success on aptos-node-v1.7.3 ==> 58449790b296a56ca0dc43f05c53bac66fc4ea3f

github-actions bot commented Nov 17, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.7.3 ==> 58449790b296a56ca0dc43f05c53bac66fc4ea3f

codecov bot commented Nov 15, 2023 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `58449790b296a56ca0dc43f05c53bac66fc4ea3f`

✅ Forge suite `compat` success on `aptos-node-v1.7.3` ==> `58449790b296a56ca0dc43f05c53bac66fc4ea3f`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.7.3` ==> `58449790b296a56ca0dc43f05c53bac66fc4ea3f`