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

fix: 9_conditional end to end test #1951

Merged
merged 13 commits into from
Jul 19, 2023
Merged

fix: 9_conditional end to end test #1951

merged 13 commits into from
Jul 19, 2023

Conversation

kevaundray
Copy link
Contributor

Description

Resolves #1802

Problem*

Resolves

Summary*

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

@kevaundray
Copy link
Contributor Author

Screenshot 2023-07-18 at 00 38 53

The above is the new error that we get due to the else branch being taken, even though a == 0

@kevaundray
Copy link
Contributor Author

kevaundray commented Jul 17, 2023

minimal circuit to reproduce:

use dep::std;



fn main(a: u32) -> pub u32{
    if a == 0 {
        3
    } else {
        1 / a
    }
}

When a == 0, we are getting a constraint failed for 1/a which should not be triggering.

@kevaundray kevaundray marked this pull request as draft July 17, 2023 23:51
@kevaundray
Copy link
Contributor Author

kevaundray commented Jul 18, 2023

checking euclidean division, the predicate is 1 (constant) which triggers the subsequent constraints.

The ssa is:

After Dead Instruction Elimination:
acir fn main f1 {
  b0(v0: u32):
    v16 = eq v0, u32 0
    enable_side_effects v16
    v17 = not v16
    enable_side_effects v17
    v18 = div u32 1, v0
    enable_side_effects u1 1
    v19 = cast v16 as u32
    v20 = cast v17 as u32
    v21 = mul v19, u32 3
    v22 = mul v20, v18
    v23 = add v21, v22
    return v23
}

Which means that the predicate should not be a constant for division. The error can then be seen in euclidean_division_var with let predicate = Expression::one();

@kevaundray
Copy link
Contributor Author

New error is because we are multiplying two Expressions which should be Linear expressions, but they are not -- we will need to reduce the lhs and rhs before multiplying

@kevaundray
Copy link
Contributor Author

kevaundray commented Jul 18, 2023

New minimal reproducable error is now:

use dep::std;

fn main(a: u32){
    if a == 0 {
  
    } else{
        let f1 = a as Field;
        assert(10/f1 == 0);
    }
}

10/f1 is failing with constraint failed -- implying that even though a == 0, the else branch is not being handled correctly

SSA for this is:

After Dead Instruction Elimination:
acir fn main f1 {
  b0(v0: u32):
    v16 = eq v0, u32 0
    enable_side_effects v16
    v17 = not v16
    enable_side_effects v17
    v18 = cast v0 as Field
    v19 = div Field 10, v18
    v20 = eq v19, Field 0
    v21 = mul v20, v17
    v22 = eq v21, v17
    constrain v22
    enable_side_effects u1 1
    return 
}

Noting that:

    enable_side_effects v16
    v17 = not v16
    enable_side_effects v17

Is somewhat confusing to read in the case that v16 is not enabled

@kevaundray
Copy link
Contributor Author

The problem; inv_var unconditionally asserts that the result has an inverse, so for the case of 1, it will always fail

@kevaundray
Copy link
Contributor Author

A bigger refactor will be needed to remove the idea of an optional predicate. Since Option is a compile time known concept, this implies that predicates are known at compile time which is not correct.

Another problem with Option or Option is that its not quite clear as to why/if None is different to Some(Expression::zero()).

@kevaundray
Copy link
Contributor Author

Blocking this PR until the issue above is created for Option<Expression/AcirVar>

@kevaundray kevaundray marked this pull request as ready for review July 18, 2023 21:32
@kevaundray kevaundray mentioned this pull request Jul 19, 2023
5 tasks
@jfecher
Copy link
Contributor

jfecher commented Jul 19, 2023

Blocking this PR until the issue above is created for Option<Expression/AcirVar>

Created #1972

@kevaundray
Copy link
Contributor Author

Blocking this PR until the issue above is created for Option<Expression/AcirVar>

Created #1972

Thanks @jfecher!

@kevaundray
Copy link
Contributor Author

@guipublic spoke offline, though could you give an official approval? @jfecher pinging for comments!

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@guipublic guipublic self-requested a review July 19, 2023 15:44
@kevaundray kevaundray added this pull request to the merge queue Jul 19, 2023
Merged via the queue into master with commit 2f6741f Jul 19, 2023
4 checks passed
@kevaundray kevaundray deleted the kw/fix-9-conditional branch July 19, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Divide by zero in 9_conditional
4 participants