chore(errors): Refactor errors representation in compiler

[yordanmadzhunkov]

Summary*

Refactoring DefCollector to return CompilationErrors

pub struct CompilationErrors {
    pub parser_errors: Vec<(ParserError, FileId)>,
    pub def_errors: Vec<(DefCollectorErrorKind, FileId)>,
    pub resolver_errors: Vec<(ResolverError, FileId)>,
    pub type_errors: Vec<(TypeCheckError, FileId)>,
}

Problem*

The motivation for this changes are:

• Allow to write an unit test that checks for specific compilation error
• Allow propagation of parsing errors from sub-modules
• Improve code readability. Now it's clear just from function signature what kind of errors the caller can expect

The refactoring of the test will be in next code review, to keep this one as short as possible

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[yordanmadzhunkov]

@jfecher Welcome back :)

[jfecher]

I've skimmed over the code and it seems reasonable. Can you elaborate more on the motivations for this PR though?

Allow to write an unit test that checks for specific compilation error

What are some examples of unit tests we'd like to write here?

Allow propagation of parsing errors from sub-modules

How so? I would think errors from sub modules are already propagated since they wouldn't be displayed at all otherwise. Is this a separate bug?

[yordanmadzhunkov]

Allow propagation of parsing errors from sub-modules -> I can't recall where I encountered this, so ignore it for now. Sorry!

Allow to write an unit test that checks for specific compilation error
One such example:

    #[test]
    fn check_trait_wrong_parameter() {
        let src = "
        trait Default {
            fn default(x: Field) -> Self;
        }
        
        struct Foo {
            bar: u32,
        }
        
        impl Default for Foo {
            fn default(x: u32) -> Self {
                Foo {bar: x}
            }
        }
        
        fn main() {
        }
        ";
        let compilator_errors = get_program_errors(src);
        //The following check will ignore warnings that are emitted by parser
        assert!(!compilator_errors.has_parser_errors()); 
        assert!(compilator_errors.resolver_errors.len() == 0);
        assert!(compilator_errors.def_errors.len() == 0);
        assert!(compilator_errors.type_errors.len() == 1);
        for err in compilator_errors.type_errors {
            match &err {
                (
                    TypeCheckError::TraitMethodParameterTypeMismatch {
                        method_name,
                        expected_typ,
                        actual_typ,
                        ..
                    },
                    _file_id,
                ) => {
                    assert_eq!(method_name, "default");
                    assert_eq!(expected_typ, "Field");
                    assert_eq!(actual_typ, "u32");
                }
                _ => {
                    assert!(
                        false,
                        "No other errors are expected in this test case! Found = {:?}",
                        err
                    );
                }
            };
        }
    }

In the old testing system, we only check if the test fails compilation, not HOW it fails to compile. This actually hide some bugs in both test code & production code. For example:

in tooling/nargo_cli/tests/compile_failure/trait_wrong_parameters_count/src/main.nr

trait Default {
    fn default(x: Field, y: Field) -> Self;
}

// This should fail to compile as `default()` should return `Foo`
impl Default for Foo {
    fn default() -> Field {
        x
    }
}

fn main() {
}

If we judge by the file name it should fail because there is wrong number of parameters.
If we judge by the comment it should fail because there is wrong return type.

Last time I checked it actually fails because x is undefined variable

An other problem I saw is in this example:

    #[test]
    fn unresolved_path() {
        let src = "
            fn main(x : Field) {
                let _z = some::path::to::a::func(x);
            }
        ";

        let mut errors = resolve_src_code(src, vec!["main", "foo"]);
        assert_eq!(errors.len(), 1);
        let err = errors.pop().unwrap();

        path_unresolved_error(err, "func");
    }

this tests uses resolve_src_code to mimic compilation behavior. When I refactored the test to share the same code as the compiler the unresolved path became from "func" to "some". In my understanding the correct one should be "some" because it's the begging of the path that can't be resolved. This actually shows that this test does not test the production code, but it actually tested the compilation mimic function resolve_src_code

After this changes are merged into the master I have prepared an other set of PRs that rewrite the tests.

[jfecher]

I don't want to block this anymore, but eventually we should reconsider storing the errors in a tuple of (error, file). As it is now there are a lot more calls to extend which obscure the happy path of the code more.

[yordanmadzhunkov]

@jfecher
I had to rebase to trigger an other build on ubuntu ( with actually failed and blocked the merge)