fix: catch unbound type variables during frontend compilation

[asterite]

Description

Problem

Resolves #7889
Resolves #8464

Summary

I started implementing this a few times until I landed with the current implementation. At first I thought we'd need to track all type variables, but for example when you have fn foo<T>() { ... }, that T in that function will remain unbound. It's only when it's called that it will become bound. This is why we can't track all type variables.

Then, I noticed that, maybe, the places where we need to track them are:

• array and slice literals (well, for the type of the array literal, and probably only when it's empty, but here it's done unconditionally)
• struct constructors, as the struct generics, if any, must be determined at the end of the function
• identifiers in expressions, corresponding to function calls and enum variants

But let me know if this isn't right.

Then, many errors started to show up. The reason is that we'd previously error during monomorphization. That means that if there was such error in a function that wasn't reached from main, that error never showed up. Now it does! And this is why tests will have a few more errors now. And, for example, there was a #[test] on top of a function were generics couldn't be deduced. That was previously only erroring when that test function was monomorphized. Now it always errors, which makes the test impossible to write, so I removed it.

Finally, when type-checking FunctionDefinition::as_typed_expr the type variables aren't checked because they are later checked when the value is interpolated somewhere else. In reality I'm not sure as_typed_expr should be using type_check_variable_with_bindings, so let me know if it should be done in a different way.

Additional Context

I'm unsure about the name RequiredTypeVariable, happy to rename it!

Documentation

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

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

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 5ba8bd7	Previous: 2a79d90	Ratio
private-kernel-reset	8.3 s	6.842 s	1.21

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[jfecher]

At first I thought we'd need to track all type variables, but for example when you have fn foo() { ... }, that T in that function will remain unbound.

The T there shouldn't be a Type::TypeVariable at all, it should be a Type::NamedGeneric. All Type::TypeVariables should be bound by the end of the function but named generics should never be bound. I realize the naming of the TypeVariable struct confuses this a bit.

My idea of this change was that we'd have Elaborator::push_type_variable be called whenever we create a Type::TypeVariable instead of just ones that can be defaulted. Was there an issue with that approach?

[asterite]

My idea of this change was that we'd have Elaborator::push_type_variable be called whenever we create a Type::TypeVariable instead of just ones that can be defaulted. Was there an issue with that approach?

The main issue is that we also need to track how that type variable related to things in the source code to be able to give an error that's understandable. Essentially saying "Cannot deduce the type of generic parameter T of function foo" instead of just "Cannot deduce the type of a generic parameter". At the point where the function context is popped I think there's no way to recover this information.

That said, I'm still not sure tracking every type variable is needed. I think this error can only happen when generics aren't specified, and this can only happen in identifiers, constructors, and array/slice literals.

[jfecher]

@asterite you could store the source location in that type variable list. E.g. you could have an enumeration or similar which gets passed to the error message

[asterite]

@jfecher I feel like the current implementation is correct so I'm a bit hesitant to try a different approach where I'm not sure it'll improve things. I'm just wondering if there's some code (some type variables) that aren't caught in this PR. If so, I can work on fixing those with this approach and, if that doesn't work, I can try the other approach.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: fa40da3	Previous: b09073a	Ratio
test_report_zkpassport_noir-ecdsa_	126 s	92 s	1.37
test_report_zkpassport_noir_rsa_	2 s	1 s	2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[asterite]

This is ready for review again, and all external libraries in our control compile file.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 1edf754	Previous: 9580a12	Ratio
rollup-merge	0.004 s	0.003 s	1.33

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[TomAFrench]

Looks like there's some test failures now. Not sure how many are flakes but the noir-contracts issue is real.

[asterite]

the noir-contracts issue is real

Right, it seems a bunch of types were introduced with a generic N that is only used in a specific impl... I just tried to fix it but I'm not sure how to do it.

[asterite]

I think I got it to compile... I'll create a PR.

[asterite]

This is ready for review again.