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

Using an untyped global as array length crashes the compiler #6046

Closed
Thjarkgh opened this issue Sep 16, 2024 · 4 comments · Fixed by #6076
Closed

Using an untyped global as array length crashes the compiler #6046

Thjarkgh opened this issue Sep 16, 2024 · 4 comments · Fixed by #6076
Labels
bug Something isn't working

Comments

@Thjarkgh
Copy link

Aim

I want to be able to derive globals from other globals, and if I accidentially include a typo, I want a error message indicating the line with the faulty global definition.

Expected Behavior

When I derive a global from a non-existing global, I want the compiler to mark that line as an error (IntelliSense should underline it red as well)

Bug

The Noir LSP simply panics:

The application panicked (crashed).
Message: index out of bounds: the len is 4918 but the index is 18446744073709551615
Location: compiler/noirc_frontend/src/node_interner.rs:1145

To Reproduce

Did not reproduce it from scratch, but can be easily reproduced in any working project:

global SERIALIZED_OBSTACLES_ARRAY_SIZE = 3;
global SERIALIZED_ENEMY_OBSTACLES_ARRAY_SIZE = SERIALIED_OBSTACLES_ARRAY_SIZE + 1;

After a few seconds Intellisense should stop working and a popup should complain about the LSP having crashed 5 times and will not be restarted anymore. In case you are not using Intellisense:

nargo compile

This will cause Noir to panic as well and show the same error message

Workaround

None

Workaround Description

No response

Additional Context

Very annoying, as it is really hard to guess what caused the panic. The missing Z in the above example was really hard to spot. I only noticed it after checking the node_interner.rs file in the Noir github. Only thanks to the compiler source code I was able to find out that I should be looking for an invalid global definition. An error, at least indicating that a global definition caused this would help a lot!

Project Impact

Nice-to-have

Blocker Context

No response

Nargo Version

noirc version = 0.34.0+359caafac5e489901d9ff02b08d1a688178d9b0a

NoirJS Version

0.32.0

Proving Backend Tooling & Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@Thjarkgh Thjarkgh added the bug Something isn't working label Sep 16, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Sep 16, 2024
@asterite
Copy link
Collaborator

Thank you for the bug report.

I tried it with the program you mention:

global SERIALIZED_OBSTACLES_ARRAY_SIZE = 3;
global SERIALIZED_ENEMY_OBSTACLES_ARRAY_SIZE = SERIALIED_OBSTACLES_ARRAY_SIZE + 1;

and there is no crash.

I think I know why the crash might happen, but it might be easier to fix it if you could provide the exact code that triggers the crash.

Thank you!

@Thjarkgh
Copy link
Author

Well, the total code is a little long - you can clone it from: https://github.com/Thjarkgh/skirmish

compile it and see that it works

then go to circuit/src/main.nr

remove the Z in line 111 (as reported in the original bug report)
nargo build
get the panic

add the Z again
notice that everything works just fine again

@asterite
Copy link
Collaborator

Thanks! I was able to reproduce the issue. Here's a code that reproduces it:

global BAR = OOPS;
global X: [Field; BAR] = [];

I'll send a fix soon :-)

@asterite asterite changed the title Deriving a global from a non-existing global causes LSP panic Using an untyped global as array length crashes the compiler Sep 18, 2024
@Thjarkgh
Copy link
Author

Ah! Yes makes sense, has to be used as an array size, that it? Yes, defined all these things as globals to be able to later on change it more easily (some of these array sizes are subject to change in future versions - as long as everything goes as planed). Thanks for your effort!

github-merge-queue bot pushed a commit that referenced this issue Sep 18, 2024
# Description

## Problem

Resolves #6046

## Summary

Another case of an unhandled`DefinitionId::dummy()`.

## 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.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Sep 18, 2024
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
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants