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

feat: Implement type aliases #2003

Closed
wants to merge 30 commits into from

Conversation

Ethan-000
Copy link
Contributor

@Ethan-000 Ethan-000 commented Jul 22, 2023

Description

Problem*

Resolves #735

Summary*

following #817

also added support for generics

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.

@Ethan-000 Ethan-000 mentioned this pull request Jul 22, 2023
6 tasks
@kevaundray kevaundray requested review from jfecher and vezenovm July 23, 2023 16:51
@@ -159,6 +168,7 @@ impl DefCollector {

// Must resolve structs before we resolve globals.
resolve_structs(context, def_collector.collected_types, crate_id, errors);
resolve_type_aliases(context, def_collector.collected_type_aliases, crate_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

The ordering here means we cannot use type aliases inside of struct definitions. While this is valid Rust:

type Bar = u32;

struct myStruct {
    foo: Bar,
}

fn main() {
    let s = myStruct { foo: 10u32 };
    println!("{}", s.foo);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is name resolution whereas collecting names should be an earlier step before this. We should double check that this use case is working as expected though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but if you have a Noir program with the same struct as above:

type Bar = Field;

struct myStruct {
    foo: Bar,
}

you will fail during resolution. When you go to resolve Bar which is a named type, the get_type_alias method will be called and panic with this:

Message:  no entry found for key
Location: crates/noirc_frontend/src/node_interner.rs:560

as the code only pushes to the type_alias map during resolve_type_aliases: https://github.com/noir-lang/noir/pull/2003/files#r1272800709.

With the way resolve_type_aliases is written now the above snippet will pass when it is called before resolve_structs.

crates/noirc_frontend/src/ast/type_alias.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/resolution/resolver.rs Outdated Show resolved Hide resolved
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.

Looks pretty good, I think there are just some smaller changes needed. Most are renamings/style suggestions.

crates/noirc_frontend/src/ast/type_alias.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/def_collector/dc_crate.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/ast/type_alias.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/def_collector/dc_crate.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/resolution/import.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir_def/types.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/node_interner.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/node_interner.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/node_interner.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/parser/mod.rs Outdated Show resolved Hide resolved
@Ethan-000
Copy link
Contributor Author

#2003 (comment)

#2003 (comment)

#2003 (comment)

There are the issues remaining

For the second one, I don't know if theres a way to solve it 🥲. open to suggestions and ideas.
cus each time resolve_type_aliases(unresolved_typ.type_alias_def) is called, it has to resolve to a Type and type Three = Two<u8> definitions are not in order so if it tries to solve this before Two<u8> is solved it won't be able to do it.

For the third one, Struct seems to be treated as a module, not sure how to do it similarly with TypeAlias

@kevaundray kevaundray requested a review from jfecher July 26, 2023 00:03
@Ethan-000
Copy link
Contributor Author

huh .. its working today
no idea why

@vezenovm
Copy link
Contributor

huh .. its working today
no idea why

Did you test it before pushing the empty definition inside the collector rather than the resolver?

@Ethan-000
Copy link
Contributor Author

huh .. its working today
no idea why

Did you test it before pushing the empty definition inside the collector rather than the resolver?

seems to be solved by this 649ed8a

I didn't change it to Type::Error to solve the issue though. just tried it randomly and it worked 🫠

@jfecher
Copy link
Contributor

jfecher commented Jul 26, 2023

@Ethan-000 maybe its a result of resolution order being random (#1122)?

It might fail some times and succeed on others.

@Ethan-000
Copy link
Contributor Author

@Ethan-000 maybe its a result of resolution order being random (#1122)?

It might fail some times and succeed on others.

yep the resolution being random is causing the problem
ideally it should solve One first then Two and Three
solving Three before One caused Three to be solved as the dummy Type put into the empty definition

I was expecting changing it to Type::Error will result Three to be solved to Type::Error if it is solved before One but its working now

@jfecher
Copy link
Contributor

jfecher commented Jul 26, 2023

I think I'm alright with merging this with the random struct/type order bug since that is an already-filed and separate issue from type aliases.

crates/noirc_frontend/src/hir/def_collector/dc_mod.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/resolution/resolver.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/node_interner.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/node_interner.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/node_interner.rs Outdated Show resolved Hide resolved
Comment on lines 399 to 412
if let Some(type_alias_type) = self.lookup_type_alias(path.clone()) {
let mut args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables));
let expected_generic_count = type_alias_type.generics.len();

if args.len() != expected_generic_count {
self.push_err(ResolverError::IncorrectGenericCount {
span,
struct_type: type_alias_type.to_string(),
actual: args.len(),
expected: expected_generic_count,
});

// Fix the generic count so we can continue typechecking
args.resize_with(expected_generic_count, || Type::Error);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here

Copy link
Contributor

@jfecher jfecher Jul 31, 2023

Choose a reason for hiding this comment

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

You have self and thus a mutable reference to the dfg so you can use that to return a mutable reference to the type alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't seem to make this work. if I understand correctly, it says |arg| already takes mutable reference and type_alias_type.to_string(), is taking it twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

closure requires unique access to `*self` but it is already borrowed
closure construction occurs hererustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20%5B2%5D?2#file%3A%2F%2F%2FUsers%2Fethan000%2Fcode%2Faztec%2Fforks%2Fnoir%2Fcrates%2Fnoirc_frontend%2Fsrc%2Fhir%2Fresolution%2Fresolver.rs)
resolver.rs(401, 40): borrow occurs here
resolver.rs(402, 47): second borrow occurs due to use of `*self` in closure
resolver.rs(403, 42): first borrow later used here

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't borrow self mutably and immutably, so there are two approaches you could take and both are different methods of changing it to not borrow self. The first is that you could replace self in the closure with this and pass this in as a parameter. The second approach would be to make it into a free function rather than a closure. I think there are few enough closed-over values that this may be the cleaner approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok this error message is not super useful. I think the problem is that args is part of type alias that the function is trying to solve and self.lookup_type_alias(path.clone()) takes the same type_aliase so if a reference is used self.resolve_type_inner(arg, new_variables) will take ownership and type_alias_type.to_string() can't use the reference. uhh don't know if I understand this correctly or not. do u mind modifying it directly to the code you are thinking if theres are better way to do this 🥲

Copy link
Contributor

Choose a reason for hiding this comment

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

I've reworked it but can't push the changes to this PR (or suggest them - since it is across multiple files), so I've opened a new PR here: #2112

@Ethan-000 Ethan-000 requested a review from jfecher July 26, 2023 18:10
crates/noirc_frontend/src/hir/resolution/resolver.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir_def/types.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/node_interner.rs Show resolved Hide resolved
@jfecher jfecher mentioned this pull request Aug 1, 2023
5 tasks
@jfecher
Copy link
Contributor

jfecher commented Aug 1, 2023

I've opened #2112 to address the comments on this PR

@jfecher jfecher closed this Aug 1, 2023
@Ethan-000
Copy link
Contributor Author

Many thanks for the review and help ^

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.

Add type aliases
3 participants