feat: type alias for numeric generics #7583

guipublic · 2025-03-05T11:22:39Z

Description

Problem*

Resolves #7272

Summary*

Allow type alias to reference numeric generic expression.

Additional Context

Changes are minimal, more use cases need to be tested. In particular there is no documentation. It can be added as a subsequent PR (or even in this one).
The PR is done in order to validate the approach taken.

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.

guipublic · 2025-03-05T14:31:49Z

I am not sure why I had to update some unit tests.

compiler/noirc_frontend/src/parser/parser/type_alias.rs

compiler/noirc_frontend/src/ast/type_alias.rs

compiler/noirc_frontend/src/tests.rs

compiler/noirc_frontend/src/ast/type_alias.rs

compiler/noirc_frontend/src/tests.rs

github-actions

⚠️ Performance Alert ⚠️

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

Benchmark suite	Current: `02fe712`	Previous: `d8ea3a4`	Ratio
`zkemail_zkemail.nr_lib`	`3` s	`1` s	`3`

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

CC: @TomAFrench

compiler/noirc_frontend/src/tests.rs

…rals

compiler/noirc_frontend/src/tests.rs

TomAFrench · 2025-03-18T16:42:20Z

tooling/lsp/src/with_file.rs

+fn noir_type_alias_with_file(noir_type_alias: TypeAlias, file: FileId) -> TypeAlias {
+    type_alias_with_file(noir_type_alias, file)
+}


Is this necessary? This function is private so we should be able to update all the references to it pretty cleanly.

I believe it was needed when I used an enum to distinguish numeric type aliases, but it's probably not needed anymore, indeed. I will check.

Lets remove this before merging

compiler/noirc_frontend/src/tests/aliases.rs

TomAFrench · 2025-03-18T17:28:27Z

compiler/noirc_frontend/src/elaborator/mod.rs

+        let (typ, num_expr) = if let Some(num_type) = alias.type_alias_def.numeric_type {
+            let num_type = self.resolve_type(num_type);
+            let kind = Kind::numeric(num_type);
+            let num_expr = if let UnresolvedTypeData::Expression(expr) =


This looks to be the issue with the identity alias. This is a pretty fragile check it seems as we can avoid it by just ensuring that the last step is an expression and it doesn't look deeper?

type Quadruple<let N: u32>: u32 = Double<N> + Double<N>;

I added a stronger test which only authorised numeric expressions for the type alias to reference.
Such type aliases did cause stack overflow during resolution so I had to return an error type.

TomAFrench · 2025-03-19T19:57:29Z

I'm a little sus by how loose we're being with the various kinds of type aliases now.

e.g. Foo is now considered an arithmetic generic type alias by the compiler.

type Foo: u32 = u32;

This errors with

error: Cannot use a type alias inside a type alias
  ┌─ src/main.nr:6:1
  │
6 │ type Foo: Bar = u32;
  │ -------------------
  │

but really we should be rejecting it as an invalid type alias off the bat.

TomAFrench · 2025-03-19T20:01:38Z

Can you add some unit tests for the cases where composing numeric type aliases results in issues? Ideally we'd have tests that we can use to determine when we can remove the block on nested numeric type aliases

jfecher

A couple more things I found before we merge.

jfecher · 2025-03-20T16:20:41Z

compiler/noirc_frontend/src/tests/visibility.rs

@@ -22,7 +22,7 @@ fn errors_if_type_alias_aliases_more_private_type() {
    let src = r#"
    struct Foo {}
    pub type Bar = Foo;
-                   ^^^ Type `Foo` is more private than item `Bar`
+    ^^^^^^^^^^^^^^^^^^ Type `Foo` is more private than item `Bar`


Can these locations be changed back to just the item? As is now we're highlighting the entire line which I don't think is as helpful

jfecher · 2025-03-20T16:21:34Z

tooling/lsp/src/with_file.rs

+fn noir_type_alias_with_file(noir_type_alias: TypeAlias, file: FileId) -> TypeAlias {
+    type_alias_with_file(noir_type_alias, file)
+}


Lets remove this before merging

jfecher · 2025-03-20T16:22:30Z

compiler/noirc_frontend/src/tests/aliases.rs

+}
+
+#[test]
+fn disallows_numeric_type_aliases_to_expression_with_alias_1() {


Suggested change

fn disallows_numeric_type_aliases_to_expression_with_alias_1() {

fn disallows_numeric_type_aliases_to_expression_with_alias_2() {

jfecher · 2025-03-20T16:23:13Z

compiler/noirc_frontend/src/tests/aliases.rs

+    type Quadruple<let N: u32>: u32 = N*(Double::<N>+3);
+    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  Cannot use a type alias inside a type alias


Can this location be changed so that it is just on Double::<N>?

jfecher · 2025-03-20T16:25:37Z

compiler/noirc_frontend/src/hir/resolution/errors.rs

+                                let mut diag = Diagnostic::simple_error(
+                                    format!("duplicate definitions of {name} found"),
+                                    "second definition found here".to_string(),
+                                    *second_location,


Looks like the formatter broke in this file? Not sure what happened here

jfecher · 2025-03-20T16:27:22Z

compiler/noirc_frontend/src/elaborator/types.rs

+    pub(crate) fn resolve_type_with_kind(&mut self, typ: UnresolvedType, kind: &Kind) -> Type {
+        let location = typ.location;
+        let resolved_type = self.resolve_type_inner(typ, kind);
+        if resolved_type.is_nested_slice() {
+            self.push_err(ResolverError::NestedSlices { location });
+        }
+        resolved_type
+    }
+


Was this added from this PR? We used to have this method although it looks like it was removed a while back. Just flagging this in case it was the result of a merge and we don't need it any more.

jfecher · 2025-03-20T16:30:09Z

compiler/noirc_frontend/src/elaborator/scope.rs

+                    // Type alias to numeric generics are aliases to some global value
+                    // Therefore we allow this case although we cannot provide the value yet
+                    return Ok((DefinitionId::dummy_id(), item));


This looks potentially suspicious - when are these dummy ids filled in? If we assign each numeric type alias a DefinitionId we can probably do away with the dummy ids here.

jfecher · 2025-03-20T16:33:55Z

compiler/noirc_frontend/src/elaborator/patterns.rs

+        if expr.id == DefinitionId::dummy_id() {
+            if let Some(PathResolutionItem::TypeAlias(alias)) = item {


Is the outer == dummy_id check necessary here? Seems like all numeric type aliases are expected to have that ID anyway and only checking for PathResolutionItem::TypeAlias would be more clear I think.

jfecher · 2025-03-21T14:41:47Z

compiler/noirc_frontend/src/ast/mod.rs

+        }
+    }
+
+    fn operator_to_binary_op_kind_helper(op: &BinaryTypeOperator) -> BinaryOpKind {


This should be a method on BinaryTypeOperator

jfecher · 2025-03-21T14:43:53Z

compiler/noirc_frontend/src/ast/mod.rs

+
+    pub(crate) fn is_valid_expression(&self) -> bool {
+        match self {
+            UnresolvedTypeExpression::Variable(path) => path.is_ident(),


The comment in elaborator/mod.rs mentions this function should allow globals & constants. We can have multiple segment paths that still resolve to constants or globals. E.g. crate::CONSTANT or foo::bar::my_global, but this method disallows these.

guipublic and others added 2 commits March 5, 2025 11:17

type alias for numeric generics

a1b9b69

Merge branch 'master' into gd/issue_7272

0c2f4ff

guipublic requested a review from a team March 5, 2025 11:23

guipublic changed the title ~~type alias for numeric generics~~ feat: type alias for numeric generics Mar 5, 2025

guipublic and others added 2 commits March 5, 2025 14:30

fix unit tests

82d639e

Merge branch 'master' into gd/issue_7272

ff24516

asterite reviewed Mar 5, 2025

View reviewed changes

compiler/noirc_frontend/src/parser/parser/type_alias.rs Outdated Show resolved Hide resolved

asterite reviewed Mar 5, 2025

View reviewed changes

compiler/noirc_frontend/src/ast/type_alias.rs Outdated Show resolved Hide resolved

asterite reviewed Mar 5, 2025

View reviewed changes

compiler/noirc_frontend/src/ast/type_alias.rs Outdated Show resolved Hide resolved

jfecher reviewed Mar 5, 2025

View reviewed changes

compiler/noirc_frontend/src/tests.rs Outdated Show resolved Hide resolved

guipublic and others added 2 commits March 11, 2025 10:55

use the aliased expression in loop bounds

4d4dabe

Merge branch 'master' into gd/issue_7272

531467a

github-actions bot reviewed Mar 11, 2025

View reviewed changes

guipublic added 3 commits March 11, 2025 11:30

code review: get location before parsing semi-column

8f50d5e

code review

249f71b

Code Review: use option instead of an enum

ca3298f

TomAFrench reviewed Mar 11, 2025

View reviewed changes

compiler/noirc_frontend/src/tests.rs Outdated Show resolved Hide resolved

guipublic and others added 11 commits March 11, 2025 17:02

Code review

7884d9f

Merge branch 'master' into gd/issue_7272

e19f7e3

rename typealias

6739cc9

Merge branch 'master' into gd/issue_7272

7b79b8a

Disable numeric type alias composition

c5e1dcc

Merge branch 'master' into gd/issue_7272

0074169

remove 'custom errors span' in test as it conflict with negative lite…

44abde3

…rals

Merge branch 'master' into gd/issue_7272

e04ad34

format

97b7063

remove custom error spans from test, completely

faf9f0c

add integration test

efcecff

guipublic and others added 7 commits March 17, 2025 11:43

Merge branch 'master' into gd/issue_7272

e1552b4

format numeric type alias

a4ae121

Merge branch 'master' into gd/issue_7272

e5de933

Merge branch 'master' into gd/issue_7272

7522b3a

chore: avoid unnecessary mutable sets of num_expr

fd35270

chore: fix me breaking things

fc7b048

chore: move tests to aliases module and add identity test

58b39a9

TomAFrench reviewed Mar 18, 2025

View reviewed changes

guipublic and others added 6 commits March 18, 2025 17:58

code review

1744fea

Merge branch 'master' into gd/issue_7272

3eb7c2f

Code review, allow variable as numeric expression

cb8d263

Merge branch 'master' into gd/issue_7272

44cd174

Strengthen error on type alias referencing other type alias

d67a9cb

Merge branch 'master' into gd/issue_7272

fcc9c04

guipublic and others added 4 commits March 20, 2025 09:37

fix composing numeric type aliases

49d865f

Code review: more specific errors and more unit tests

fb837af

Merge branch 'master' into gd/issue_7272

afa5cc9

Merge branch 'master' into gd/issue_7272

02fe712

jfecher reviewed Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: type alias for numeric generics #7583

feat: type alias for numeric generics #7583

guipublic commented Mar 5, 2025

guipublic commented Mar 5, 2025

github-actions bot left a comment •

edited

Loading

TomAFrench Mar 18, 2025

guipublic Mar 19, 2025

jfecher Mar 20, 2025

TomAFrench Mar 18, 2025

guipublic Mar 19, 2025

TomAFrench commented Mar 19, 2025

TomAFrench commented Mar 19, 2025

jfecher left a comment

jfecher Mar 20, 2025

jfecher Mar 20, 2025

jfecher Mar 20, 2025

jfecher Mar 20, 2025

jfecher Mar 20, 2025

jfecher Mar 20, 2025

jfecher Mar 20, 2025

jfecher Mar 20, 2025

jfecher Mar 21, 2025

jfecher Mar 21, 2025

	fn disallows_numeric_type_aliases_to_expression_with_alias_1() {
	fn disallows_numeric_type_aliases_to_expression_with_alias_2() {

		type Quadruple<let N: u32>: u32 = N*(Double::<N>+3);
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Cannot use a type alias inside a type alias

		if expr.id == DefinitionId::dummy_id() {
		if let Some(PathResolutionItem::TypeAlias(alias)) = item {

feat: type alias for numeric generics #7583

Are you sure you want to change the base?

feat: type alias for numeric generics #7583

Conversation

guipublic commented Mar 5, 2025

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

guipublic commented Mar 5, 2025

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAFrench commented Mar 19, 2025

TomAFrench commented Mar 19, 2025

jfecher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment •

edited

Loading