Skip to content

Commit

Permalink
fix!: Check unused generics are bound (#5840)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

## Summary\*

Ran into this while working on arithmetic generics. It's possible to
have cases like:

```rs
fn foo<let N: u32>() {}

fn main() {
    foo();
}
```

For which `N` is unused but we don't issue a "type annotations needed"
error for it currently because it isn't present in the instantiated
function type of `foo`. This PR fixes this and issues the error.

## Additional Context

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
jfecher authored Aug 27, 2024
1 parent 3c778b7 commit 82eb158
Show file tree
Hide file tree
Showing 10 changed files with 54 additions and 33 deletions.
8 changes: 8 additions & 0 deletions compiler/noirc_frontend/src/monomorphization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,14 @@ impl<'interner> Monomorphizer<'interner> {
return self.resolve_trait_method_expr(expr_id, typ, method);
}

// Ensure all instantiation bindings are bound.
// This ensures even unused type variables like `fn foo<T>() {}` have concrete types
if let Some(bindings) = self.interner.try_get_instantiation_bindings(expr_id) {
for (_, binding) in bindings.values() {
Self::check_type(binding, ident.location)?;
}
}

let definition = self.interner.definition(ident.id);
let ident = match &definition.kind {
DefinitionKind::Function(func_id) => {
Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1289,6 +1289,10 @@ impl NodeInterner {
&self.instantiation_bindings[&expr_id]
}

pub fn try_get_instantiation_bindings(&self, expr_id: ExprId) -> Option<&TypeBindings> {
self.instantiation_bindings.get(&expr_id)
}

pub fn get_field_index(&self, expr_id: ExprId) -> usize {
self.field_indices[&expr_id]
}
Expand Down
18 changes: 9 additions & 9 deletions noir_stdlib/src/collections/map.nr
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,10 @@ impl<K, V> Slot<K, V> {
// While conducting lookup, we iterate attempt from 0 to N - 1 due to heuristic,
// that if we have went that far without finding desired,
// it is very unlikely to be after - performance will be heavily degraded.
impl<K, V, let N: u32, B, H> HashMap<K, V, N, B> {
impl<K, V, let N: u32, B> HashMap<K, V, N, B> {
// Creates a new instance of HashMap with specified BuildHasher.
// docs:start:with_hasher
pub fn with_hasher(_build_hasher: B) -> Self
pub fn with_hasher<H>(_build_hasher: B) -> Self
where
B: BuildHasher<H> {
// docs:end:with_hasher
Expand All @@ -99,7 +99,7 @@ impl<K, V, let N: u32, B, H> HashMap<K, V, N, B> {

// Returns true if the map contains a value for the specified key.
// docs:start:contains_key
pub fn contains_key(
pub fn contains_key<H>(
self,
key: K
) -> bool
Expand Down Expand Up @@ -183,7 +183,7 @@ impl<K, V, let N: u32, B, H> HashMap<K, V, N, B> {

// For each key-value entry applies mutator function.
// docs:start:iter_mut
pub fn iter_mut(
pub fn iter_mut<H>(
&mut self,
f: fn(K, V) -> (K, V)
)
Expand All @@ -208,7 +208,7 @@ impl<K, V, let N: u32, B, H> HashMap<K, V, N, B> {

// For each key applies mutator function.
// docs:start:iter_keys_mut
pub fn iter_keys_mut(
pub fn iter_keys_mut<H>(
&mut self,
f: fn(K) -> K
)
Expand Down Expand Up @@ -278,7 +278,7 @@ impl<K, V, let N: u32, B, H> HashMap<K, V, N, B> {

// Get the value by key. If it does not exist, returns none().
// docs:start:get
pub fn get(
pub fn get<H>(
self,
key: K
) -> Option<V>
Expand Down Expand Up @@ -313,7 +313,7 @@ impl<K, V, let N: u32, B, H> HashMap<K, V, N, B> {

// Insert key-value entry. In case key was already present, value is overridden.
// docs:start:insert
pub fn insert(
pub fn insert<H>(
&mut self,
key: K,
value: V
Expand Down Expand Up @@ -356,7 +356,7 @@ impl<K, V, let N: u32, B, H> HashMap<K, V, N, B> {

// Removes a key-value entry. If key is not present, HashMap remains unchanged.
// docs:start:remove
pub fn remove(
pub fn remove<H>(
&mut self,
key: K
)
Expand Down Expand Up @@ -388,7 +388,7 @@ impl<K, V, let N: u32, B, H> HashMap<K, V, N, B> {
}

// Apply HashMap's hasher onto key to obtain pre-hash for probing.
fn hash(
fn hash<H>(
self,
key: K
) -> u32
Expand Down
22 changes: 11 additions & 11 deletions noir_stdlib/src/collections/umap.nr
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ impl<K, V> Slot<K, V> {
// While conducting lookup, we iterate attempt from 0 to N - 1 due to heuristic,
// that if we have went that far without finding desired,
// it is very unlikely to be after - performance will be heavily degraded.
impl<K, V, B, H> UHashMap<K, V, B> {
impl<K, V, B> UHashMap<K, V, B> {
// Creates a new instance of UHashMap with specified BuildHasher.
// docs:start:with_hasher
pub fn with_hasher(_build_hasher: B) -> Self
pub fn with_hasher<H>(_build_hasher: B) -> Self
where
B: BuildHasher<H> {
// docs:end:with_hasher
Expand All @@ -88,7 +88,7 @@ impl<K, V, B, H> UHashMap<K, V, B> {
Self { _table, _len, _build_hasher }
}

pub fn with_hasher_and_capacity(_build_hasher: B, capacity: u32) -> Self
pub fn with_hasher_and_capacity<H>(_build_hasher: B, capacity: u32) -> Self
where
B: BuildHasher<H> {
// docs:end:with_hasher
Expand All @@ -110,7 +110,7 @@ impl<K, V, B, H> UHashMap<K, V, B> {

// Returns true if the map contains a value for the specified key.
// docs:start:contains_key
pub fn contains_key(
pub fn contains_key<H>(
self,
key: K
) -> bool
Expand Down Expand Up @@ -194,7 +194,7 @@ impl<K, V, B, H> UHashMap<K, V, B> {

// For each key-value entry applies mutator function.
// docs:start:iter_mut
unconstrained pub fn iter_mut(
unconstrained pub fn iter_mut<H>(
&mut self,
f: fn(K, V) -> (K, V)
)
Expand All @@ -216,7 +216,7 @@ impl<K, V, B, H> UHashMap<K, V, B> {

// For each key applies mutator function.
// docs:start:iter_keys_mut
unconstrained pub fn iter_keys_mut(
unconstrained pub fn iter_keys_mut<H>(
&mut self,
f: fn(K) -> K
)
Expand Down Expand Up @@ -283,7 +283,7 @@ impl<K, V, B, H> UHashMap<K, V, B> {

// Get the value by key. If it does not exist, returns none().
// docs:start:get
unconstrained pub fn get(
unconstrained pub fn get<H>(
self,
key: K
) -> Option<V>
Expand Down Expand Up @@ -315,7 +315,7 @@ impl<K, V, B, H> UHashMap<K, V, B> {

// Insert key-value entry. In case key was already present, value is overridden.
// docs:start:insert
unconstrained pub fn insert(
unconstrained pub fn insert<H>(
&mut self,
key: K,
value: V
Expand Down Expand Up @@ -353,7 +353,7 @@ impl<K, V, B, H> UHashMap<K, V, B> {
}
}

unconstrained fn try_resize(&mut self)
unconstrained fn try_resize<H>(&mut self)
where B: BuildHasher<H>, K: Eq + Hash, H: Hasher {
if self.len() + 1 >= self.capacity() / 2 {
let capacity = self.capacity() * 2;
Expand All @@ -368,7 +368,7 @@ impl<K, V, B, H> UHashMap<K, V, B> {

// Removes a key-value entry. If key is not present, UHashMap remains unchanged.
// docs:start:remove
unconstrained pub fn remove(
unconstrained pub fn remove<H>(
&mut self,
key: K
)
Expand Down Expand Up @@ -397,7 +397,7 @@ impl<K, V, B, H> UHashMap<K, V, B> {
}

// Apply UHashMap's hasher onto key to obtain pre-hash for probing.
fn hash(
fn hash<H>(
self,
key: K
) -> u32
Expand Down
9 changes: 3 additions & 6 deletions noir_stdlib/src/hash/sha256.nr
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ pub fn sha256_var<let N: u32>(msg: [u8; N], message_size: u64) -> [u8; 32] {
msg_block[msg_byte_ptr] = 1 << 7;
msg_byte_ptr = msg_byte_ptr + 1;
unsafe {
let (new_msg_block, new_msg_byte_ptr)= pad_msg_block(msg_block, msg_byte_ptr);
let (new_msg_block, new_msg_byte_ptr) = pad_msg_block(msg_block, msg_byte_ptr);
msg_block = new_msg_block;
if crate::runtime::is_unconstrained() {
msg_byte_ptr = new_msg_byte_ptr;
Expand Down Expand Up @@ -188,10 +188,7 @@ pub fn sha256_var<let N: u32>(msg: [u8; N], message_size: u64) -> [u8; 32] {
hash_final_block(msg_block, h)
}

unconstrained fn pad_msg_block<let N: u32>(
mut msg_block: [u8; 64],
mut msg_byte_ptr: u64
) -> ([u8; 64], u64) {
unconstrained fn pad_msg_block(mut msg_block: [u8; 64], mut msg_byte_ptr: u64) -> ([u8; 64], u64) {
// If i >= 57, there aren't enough bits in the current message block to accomplish this, so
// the 1 and 0s fill up the current block, which we then compress accordingly.
if msg_byte_ptr >= 57 {
Expand All @@ -208,7 +205,7 @@ unconstrained fn pad_msg_block<let N: u32>(
(msg_block, msg_byte_ptr)
}

unconstrained fn attach_len_to_msg_block<let N: u32>(
unconstrained fn attach_len_to_msg_block(
mut msg_block: [u8; 64],
mut msg_byte_ptr: u64,
message_size: u64
Expand Down
2 changes: 1 addition & 1 deletion noir_stdlib/src/option.nr
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ impl<T> Option<T> {
}

/// If self is Some, return self. Otherwise, return `default()`.
pub fn or_else<U, Env>(self, default: fn[Env]() -> Self) -> Self {
pub fn or_else<Env>(self, default: fn[Env]() -> Self) -> Self {
if self._is_some { self } else { default() }
}

Expand Down
7 changes: 7 additions & 0 deletions test_programs/compile_failure/unspecified_generic/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "unspecified_generic"
type = "bin"
authors = [""]
compiler_version = ">=0.33.0"

[dependencies]
5 changes: 5 additions & 0 deletions test_programs/compile_failure/unspecified_generic/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
fn foo<T>() {}

fn main() {
foo();
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
fn main() {
// s: Struct<?, ()>
let s = Struct { b: () };
// s: Struct<?PolymorphicIntOrField, ()>
let s = Struct { a: 0, b: () };
// Regression for #3089
s.foo();
}

struct Struct<A, B> { b: B }
struct Struct<A, B> { a: A, b: B }

// Before the fix, this candidate is searched first, binding ? to `u8` permanently.
impl<B> Struct<u8, u8> {
impl Struct<u8, u8> {
fn foo(self) {}
}

Expand All @@ -18,6 +18,6 @@ impl<B> Struct<u8, u8> {
// With the fix, the type of `s` correctly no longer changes until a
// method is actually selected. So this candidate is now valid since
// `Struct<u32, ()>` unifies with `Struct<?, ()>` with `? = u32`.
impl<A> Struct<u32, ()> {
impl Struct<u32, ()> {
fn foo(self) {}
}
2 changes: 1 addition & 1 deletion test_programs/compile_success_empty/option/src/main.nr
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
fn main() {
let ten = 10; // giving this a name, to ensure that the Option functions work with closures
let none = Option::none();
let none: Option<Field> = Option::none();
let some = Option::some(3);

assert(none.is_none());
Expand Down

0 comments on commit 82eb158

Please sign in to comment.