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

chore(docs): correction of data type of loop index #3609

Closed
wants to merge 1 commit into from

Conversation

TAdev0
Copy link
Contributor

@TAdev0 TAdev0 commented Nov 28, 2023

Description

Problem*

Resolves #3315

Summary*

This PR replaces u64 for field in the description of the loop index i.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

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

@@ -19,7 +19,7 @@ for i in 0..10 {
};
```

The index for loops is of type `u64`.
The index for loops is of type `field`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We changed this to be u64 -- do you see something different when you run Noir programs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevaundray yes, even with 0.19.2, i get this error :

error: Comparisons are invalid on Field types. Try casting the operands to a sized integer type first

if i do something like this :

    for i in 0..8 {
        for j in 0..8 {                   
             if  (i + j < 8) {...}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevaundray oh i see, 8 is of type field, this code works like this :
if (i+j < 8 as u64) {...}

my bad!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, actually, the behaviour is odd, this compiles as well :

  if  (i as u64 + j as u64 < 8) {...}

It seems one of the two operands needs to be converted to a uint

Copy link
Contributor

Choose a reason for hiding this comment

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

I think thats possibly a separate issue -- I would have expected 8 to be automatically casted to a u64 without the explicit cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that 8 is automatically casted to a u64 only if we explicitly declare i and j as u64 in the comparison.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you open an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll do it tomorrow in the morning as soon as i am available.
Should i close this PR as it is finally irrelevant ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This may be an ordering issue with two type constraints. IIRC i and j are not actually u64s, they just default to u64 if no other type is specified. The same is the case for the literal 8, although it defaults to Field instead.

The important part is that internally we have a TypeVariableKind::IntegerOrField (for 8) but not a corresponding kind for just integer / u64. The delayed_type_check hack is used instead which is why this defaults to Field and is not caught. The fix for this is to remove the hack and have a dedicated type variable kind - so yes this should be a separate issue. This PR can be closed.

@jfecher
Copy link
Contributor

jfecher commented Nov 28, 2023

Closing this - it is part of a separate issue.

@jfecher jfecher closed this Nov 28, 2023
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.

Error in the "Control Flow" section of Noir documentation
3 participants