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: multiple item imports in use statement #1466

Merged
merged 6 commits into from Jun 5, 2023
Merged

feat: multiple item imports in use statement #1466

merged 6 commits into from Jun 5, 2023

Conversation

ghost
Copy link

@ghost ghost commented May 31, 2023

Description

Implement multiple item imports in use statement: use foo::{bar, hello}.

Summary

Added support for multiple item imports in the use statement, allowing for more flexible and targeted module imports. Updated the parser, AST, and desugaring logic to handle the new syntax.

Additional Context

PR Checklist

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

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.

This solution looks pretty good and is quite simple because of the desugaring. I do think we should parse UseTrees recursively which should clean up the parsing code a little and allow us to support recursive { and } in imports.

crates/noirc_frontend/src/ast/statement.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/parser/parser.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.

This PR looks very good. I did run into some minor usability concerns while testing it though. Once we fix these last issues I think it is all set!

crates/noirc_frontend/src/parser/parser.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/parser/parser.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/parser/parser.rs Show resolved Hide resolved
@jfecher
Copy link
Contributor

jfecher commented Jun 5, 2023

Closing this PR, it is superseded by #1548

@jfecher jfecher closed this Jun 5, 2023
@jfecher
Copy link
Contributor

jfecher commented Jun 5, 2023

Reviewing this PR I got into a bit of a rabbit hole with handling the cases for use dep::dep::dep, nested blocks use {{{{foo}}}};, and their implications. I now think we should just merge this PR and we can look to fix the use dep::dep::dep; and other corner cases later since they will rarely be encountered in actual code anyway.

@jfecher jfecher reopened this Jun 5, 2023
jfecher
jfecher previously approved these changes Jun 5, 2023
@jfecher jfecher enabled auto-merge June 5, 2023 18:54
@jfecher
Copy link
Contributor

jfecher commented Jun 5, 2023

Looks like clippy caught some code which can be simplified

auto-merge was automatically disabled June 5, 2023 19:08

Head branch was pushed to by a user without write access

@jfecher jfecher enabled auto-merge June 5, 2023 19:12
@jfecher jfecher added this pull request to the merge queue Jun 5, 2023
Merged via the queue into noir-lang:master with commit 1dcd2ee Jun 5, 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.

1 participant