Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

for-each sugar does not work in the standard library. #618

Closed
jfecher opened this issue Jan 5, 2023 · 6 comments
Closed

for-each sugar does not work in the standard library. #618

jfecher opened this issue Jan 5, 2023 · 6 comments
Labels
bug Something isn't working

Comments

@jfecher
Copy link
Contributor

jfecher commented Jan 5, 2023

Description

Aim

Using a for-each loop in a std lib file such as hash.nr:

for elem in array {
    ...
}

Expected behavior

It to loop over each element of the array.

Bug

Instead the compiler crashes claiming it cannot find the std module. This is likely due to the above loop desugaring into:

for i in std::array::len(array) {
    let elem = array[i];
    ...
}

and the standard library expects the len function to be referred to as crate::array::len rather than std::array::len.

There are two items that should be fixed here

  1. The compiler should not crash when a module is not found for import.
  2. We should allow modules to reference themselves by their own name in addition to crate. This enables us to have a non-relative module hierarchy where we do not need to change name references based on which module they are used in.
@jfecher jfecher added the bug Something isn't working label Jan 5, 2023
@kevaundray
Copy link
Contributor

Number 2 seems to be a new feature that is not in the rust language. We should investigate the available options here and how it's done in Rust.

IIUC 'crate' cannot be used in this case because the for-each sugar can be in any module.

'std' seems like it's not exactly right because now a language feature depends on the stdlib, whereas we can compile Noir code without it in the web.

@guipublic
Copy link
Contributor

May be we should have an internal crate which is part of noir and includes functions used by the language itself such as the array length.
We could define this one with ::array::len()
Only the internal noir lib will be allowed to start with ::

@jfecher
Copy link
Contributor Author

jfecher commented Jan 11, 2023

Rust allows crates to refer to themselves via their own name if there is a lib.rs file present alongside or instead of main.rs. I see no reason why we couldn't always allow this by default. I wasn't aware of compiling noir code without the standard lib (why not include it anyway?) - but even if one wanted to not include the stdlib I think builtins like std::array::len should still be included.

@kevaundray
Copy link
Contributor

Rust allows crates to refer to themselves via their own name if there is a lib.rs file present alongside or instead of main.rs.

Rust allows crates to refer to themselves via their own name if there is a lib.rs file present alongside or instead of main.rs.

When there is a main.rs alongside a lib.rs, the main.rs file can call the lib.rs but these are seen as two different crates. So main.rs won't be able to access anything that has pub(crate) visibility in lib.rs for example.

I wasn't aware of compiling noir code without the standard lib (why not include it anyway?)

You can do this in typescript but I think it worth bringing this up in our next meeting to see if we can enforce this

I think builtins like std::array::len should still be included.

I agree and maybe we will need to always include stdlib or maybe have a core like what @guipublic was suggesting which includes builtins and just has the essentials

@kevaundray
Copy link
Contributor

@jfecher whats the status of this issue?

@jfecher
Copy link
Contributor Author

jfecher commented Jul 17, 2023

This was fixed when the desugaring was changed from for i in 0 .. dep::std::array::len(array) { ... } to for i in 0 .. array.len() {. It is also currently used in our stdlib, so it is confirmed fixed now.

@jfecher jfecher closed this as completed Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants