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

Compiler raises an error when returning a negative value after a } because the compiler interprets it as a subtraction #5228

Closed
pventuzelo opened this issue Jun 12, 2024 · 0 comments · Fixed by #5634
Assignees
Labels
bug Something isn't working

Comments

@pventuzelo
Copy link

Aim

We (https://github.com/FuzzingLabs) found that the compiler raises an error when returning a negative value after a } because the compiler interprets it as a subtraction.

Expected Behavior

No error should occur.

Bug

It seems like the compiler treats the } and the return as a single statement, similar to a subtraction operation. The error occurs because the function ends up returning a subtraction between a Field and () but this is an invalid instruction. Placing a ; between the } and the return allows the compiler to properly treat the negative value as a return statement.

The issue occurs with the latest compiler version on the master branch. The error message is as follows:

error: expected type i8, found type ()
...

error: Types in a binary operation should match, but found () and Field
...

error: No matching impl found for `(): Sub
...

To Reproduce

// The compiler raises an error due to an isolated negative sign followed by a number, which is interpreted as a subtraction.
fn func1() -> i8 {
  {
  }
  -1 // This is interpreted as a subtraction and not a negative number return.
}

// Same error with "if" "else" etc... (not "for" because the loop is flattened)
fn func2() -> i8 {
  if true {
  } else {
  }
  -1 
}

// No error here because there is a ";" between the "}" and the return.
fn func3() -> i8 {
  {
  }
  ;
  -1 
}

fn main() {
}

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

None

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@pventuzelo pventuzelo added the bug Something isn't working label Jun 12, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 30, 2024
…tatements (#5634)

# Description

## Problem

Resolves #5228

## Summary

The reason why this was parsed fine:

```rust
fn foo() {
  for i in 0..10 {
  }
  -1
}
```

but this wasn't:

```rust
fn foo() {
  {}
  -1
}
```

is that a for expression is parsed as a statement choice, but a block
was just parsed as part of an expression. Because of this, if you had
`{} - 1` it was parsed as an infix expression.

Note that that infix expression is still valid if it's supposed to be an
expression, for example:

```rust
let x = { 10 } - 1;
```

However, there's a difference if that `{` appears as a stand-alone
statement: in that case it shouldn't be considered an expression that
can form part of other expressions.

The same thing was done with `if`, which before this PR could never show
up as a stand-alone statement.

I checked with Rust and this is the same there (I didn't check their
implementation, just that our behavior now matches that of Rust).

## Additional Context

This is a breaking change. If code relied on this behavior, it will now
require parentheses around it. For example, this used to work (there was
one case in our codebase):

```rust
if 1 { 2 } else { 3 } as Field
```

Now it must be written like this:

```rust
(if 1 { 2 } else { 3 }) as Field
```

But, again, this is the same as Rust.

## 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.
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