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.

Sign up for GitHub

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

Jump to bottom

fix: Differentiate stdlib CrateId from others #1895

Merged
merged 5 commits into from
Jul 18, 2023
Merged

fix: Differentiate stdlib CrateId from others #1895

merged 5 commits into from
Jul 18, 2023

Conversation

phated
Copy link
Contributor

@phated phated commented Jul 7, 2023

Description

Problem*

Resolves #1894
Towards #1838

Summary*

This removes the concept of LOCAL_CRATE being a fixed CrateId(0) and instead uses the crate id generated by adding a crate to the graph. The only place where hardcoding the value was useful was checking for impl on primitive values; however, as noted in #1894, this was a bad solution and the stdlib should be a special case of CrateId so it has become an enum and solves that bug.

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.

TomAFrench
TomAFrench reviewed Jul 8, 2023
View reviewed changes
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Nice to break out the stdlib more explicitly. Not sure we need to pass around the root crate id however.

crates/noirc_frontend/src/graph/mod.rs Show resolved Hide resolved
crates/noirc_driver/src/main.rs Outdated Show resolved Hide resolved
@TomAFrench TomAFrench changed the title fix: Differentiate stdlib CrateId from others fix: Differentiate stdlib CrateId from others Jul 10, 2023
@phated phated force-pushed the phated/remove-local-crate branch from f8ceba7 to a986162 Compare July 17, 2023 18:42
@phated phated requested a review from TomAFrench July 17, 2023 18:59
TomAFrench
TomAFrench previously approved these changes Jul 17, 2023
View reviewed changes
@kevaundray kevaundray added this pull request to the merge queue Jul 18, 2023
Merged via the queue into master with commit 211e251 Jul 18, 2023
4 checks passed
@kevaundray kevaundray deleted the phated/remove-local-crate branch July 18, 2023 07:01
@kevaundray kevaundray mentioned this pull request Jul 18, 2023
Merged
@phated phated mentioned this pull request Jul 26, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfecher jfecher jfecher left review comments

@TomAFrench TomAFrench TomAFrench approved these changes

@kevaundray kevaundray Awaiting requested review from kevaundray

@kobyhallx kobyhallx Awaiting requested review from kobyhallx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Primitive types can have impl inside dependencies
4 participants
@phated @jfecher @TomAFrench @kevaundray