Skip to content

feat: Implement type suffixes#8970

Merged
jfecher merged 14 commits intomasterfrom
jf/type-suffixes
Jun 23, 2025
Merged

feat: Implement type suffixes#8970
jfecher merged 14 commits intomasterfrom
jf/type-suffixes

Conversation

@jfecher
Copy link
Contributor

@jfecher jfecher commented Jun 18, 2025

Description

Problem*

Resolves #8759
Resolves #8968

Summary*

Does what it says on the tin. 90% of changes are .snap changes.

Additional Context

To keep nargo expand tests working, we print out the type suffix on every integer literal there since they are normally not stored in the Hir.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] 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.

@jfecher jfecher requested a review from a team June 18, 2025 18:56
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jun 18, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 18, 2025

@asterite
Copy link
Collaborator

This PR also fixes some past inconsistencies. For example this code:

fn main() {
    let x = comptime {
        let x: i8 = -1;
        x
    };
    println(x);
}

used to print "0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000000" because the value that came out of the comptime block was just "1" without any suffix, but now it's "1_u8".

On the other hand, this code used to compile:

fn foo() -> Field {
    comptime {
        let x: i8 = -1;
        x
    }
}

but now it doesn't, so this PR could be considered a breaking change (but it's fine: this is a bug fix).

All of these seem unrelated to the CI failures, though! It must be something with numeric kinds getting a type suffix or something...

@jfecher
Copy link
Contributor Author

jfecher commented Jun 18, 2025

On the other hand, this code used to compile

Yeah, locally the error I'm seeing is with comptime_return_minus_one_as_i32:

fn main() -> pub i32 {
    comptime {
        -1
    }
}

This used to compile but now actually errors with Field != i32

@asterite
Copy link
Collaborator

The errors on CI seems to be related to this not compiling:

fn main() {
    let _ = [0; 10_u32];
}

nor this:

fn main() {
    let _: [Field; 10_u32] = [0; 10];
}

(sorry if you were already investigating that, I got curious 😊)

@asterite
Copy link
Collaborator

The error you mention is also related to this issue: #8389

I wonder if we could pass down the expected type of the comptime block.

In this code:

fn main() -> pub i32 {
    comptime {
        -1
    }
}

we could pass the type i32 to the comptime block, which would act like its return type. Then -1 would get the type i32.

It would be similar in this code:

fn main() -> pub i32 {
    comptime {
        let x = -1;
        x
    }
}

I think we are already passing the target type in the elaborator, but not using it in all cases.

That said... I think comptime blocks are eagerly evaluated before the function finishes so in something like this it might not work:

fn main() -> pub i32 {
    let y = comptime {
        let x = -1;
        x
    };
    y
}

Just an idea :-)

@jfecher
Copy link
Contributor Author

jfecher commented Jun 18, 2025

Found another failure:

    trait Serialize<let N: u32> {
        fn serialize() {}
    }

    #[attr]
    pub fn foobar() {}

    comptime fn attr(_f: FunctionDefinition) -> Quoted {
        let serialized_len = 1;
        // We are testing that when we unquote $serialized_len, it's unquoted
        // as the token `1` and not as something else that later won't be parsed correctly
        // in the context of a generic argument.
        quote {
            impl Serialize<$serialized_len> for Field {
                fn serialize() { }
            }
        }
    }

    fn main() {}

This now fails where we substitute let serialized_len = 1; in for impl Serialize<$serialized_len> for Field { because Serialize expects a u32 and now with the type suffix it is seeing 1_Field

Edit: Actually we still get the same error even with specifying "_u32"

@aakoshh
Copy link
Contributor

aakoshh commented Jun 18, 2025

Could you update the monomorphized AST printer as well? 🙏

@asterite
Copy link
Collaborator

Maybe if we don't carry the suffix out of comptime code then it would work? We'd lose the fixes I mentioned above, but that's how it was working anyway.

@jfecher
Copy link
Contributor Author

jfecher commented Jun 20, 2025

@asterite yeah, it may be worth reverting that portion to get this change in. The bug in that case would still be logged to be fixed later.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 681e8b9 Previous: a56d36a Ratio
rollup-merge 0.004 s 0.003 s 1.33

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 681e8b9 Previous: a56d36a Ratio
private-kernel-tail 1.306 s 1.068 s 1.22

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@jfecher
Copy link
Contributor Author

jfecher commented Jun 20, 2025

PR should be ready for review - with the into_tokens revert and some snap file updates all tests are now passing

@jfecher jfecher enabled auto-merge June 20, 2025 22:37
Copy link
Collaborator

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Looks great! I left a couple of comments.

@jfecher jfecher added this pull request to the merge queue Jun 23, 2025
@github-actions
Copy link
Contributor

FYI @noir-lang/developerrelations on Noir doc changes.

Merged via the queue into master with commit 27e5ed3 Jun 23, 2025
128 of 131 checks passed
@jfecher jfecher deleted the jf/type-suffixes branch June 23, 2025 21:57
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 24, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: Implement type suffixes
(noir-lang/noir#8970)
fix: add parent traits when adding trait impl where clause
(noir-lang/noir#9000)
fix(fuzz): Use indirection for taking `&mut` over an array element
(noir-lang/noir#8992)
chore: remove unreachable functions after inlining simple functions
(noir-lang/noir#8987)
chore: bump external pinned commits
(noir-lang/noir#8990)
END_COMMIT_OVERRIDE

Co-authored-by: AztecBot <tech@aztecprotocol.com>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 24, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: Implement type suffixes
(noir-lang/noir#8970)
fix: add parent traits when adding trait impl where clause
(noir-lang/noir#9000)
fix(fuzz): Use indirection for taking `&mut` over an array element
(noir-lang/noir#8992)
chore: remove unreachable functions after inlining simple functions
(noir-lang/noir#8987)
chore: bump external pinned commits
(noir-lang/noir#8990)
END_COMMIT_OVERRIDE

Co-authored-by: AztecBot <tech@aztecprotocol.com>
danielntmd pushed a commit to danielntmd/aztec-packages that referenced this pull request Jul 16, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: Implement type suffixes
(noir-lang/noir#8970)
fix: add parent traits when adding trait impl where clause
(noir-lang/noir#9000)
fix(fuzz): Use indirection for taking `&mut` over an array element
(noir-lang/noir#8992)
chore: remove unreachable functions after inlining simple functions
(noir-lang/noir#8987)
chore: bump external pinned commits
(noir-lang/noir#8990)
END_COMMIT_OVERRIDE

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Variables can be lexed as part of an integer only if the variable only contains a, b, c, d, e or f Add integer type suffixes

3 participants