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

chore: Remove the concept of CrateType #1868

Closed
wants to merge 1 commit into from

Conversation

phated
Copy link
Contributor

@phated phated commented Jul 5, 2023

Description

Problem*

Towards #1838

Summary*

This removes the concept of a CrateType as it doesn't really matter except for adding dependencies and doing a "full" compilation (with backend, etc). In the cases where we need to know if something is a "binary crate" we can just check to see if it has a main function.

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.

@phated phated requested review from TomAFrench and kobyhallx July 5, 2023 21:24
@kevaundray
Copy link
Contributor

I don't understand the reason to remove this; since a binary crate and something having a main function are not the same thing.

Also curious as to how this plays out with workspaces

Comment on lines 76 to 79
pub fn is_binary_crate(&self, crate_id: &CrateId) -> bool {
// Currently, anything with a `main` function is a binary crate
matches!(self.get_main_function(crate_id), Some(_))
}
Copy link
Member

Choose a reason for hiding this comment

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

A library (lib.nr) with a main function would be treated as a binary crate here.

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 think that is fine though; although, in rust, the extra main would be ignored and clippy would give a warning that it is unused. I could see us doing that, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

This still has the concept of a crate-type in the frontend, its just that it is implicit by having a main function IIUC. I think its better to be explicit about the crate-type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy !== frontend in rust. Clippy is a linting tool that runs some phases of the compiler and then gives you feedback. The cargo abstraction defines what a lib or bin actually is, including specifying custom paths (you can specify src/lib.rs as the path for a [[bin]] iirc).

@phated
Copy link
Contributor Author

phated commented Jul 6, 2023

I don't understand the reason to remove this; since a binary crate and something having a main function are not the same thing.

From the compiler perspective, anything through the check phase doesn't care about this at all. The only thing that cares about this is dependency resolution and the final step of "compile_main". And it could be argued that it is not something that dependency resolution should care about (e.g. cargo allows a lib.rs with a main function and clippy warns you it is unused). Additionally, the only heuristic for determining this difference was the name lib.rs vs main.rs which gets really clunky when we start allowing custom paths (such as how cargo allows it).

Also curious as to how this plays out with workspaces

I'm not exactly sure how you view workspaces, as I view anything with a Nargo.toml as a workspace because it is a grouping of files; however, if there is additional logic needed inside nargo (which is the "workspace tool") then it should happen inside nargo, instead deep in the frontend which should just be compiling source code.

@phated phated force-pushed the phated/kill-driver branch from 196299f to 9417e80 Compare July 6, 2023 15:12
Base automatically changed from phated/kill-driver to master July 6, 2023 17:49
@phated phated force-pushed the phated/remove-crate-type branch from b480f01 to 60db9c6 Compare July 6, 2023 18:13
@phated phated marked this pull request as ready for review July 6, 2023 18:13
@kevaundray
Copy link
Contributor

I'm not exactly sure how you view workspaces, as I view anything with a Nargo.toml as a workspace because it is a grouping of files; however, if there is additional logic needed inside nargo (which is the "workspace tool") then it should happen inside nargo, instead deep in the frontend which should just be compiling source code.

I was using the same definition as cargo workspaces : https://doc.rust-lang.org/book/ch14-03-cargo-workspaces.html

From the compiler perspective, anything through the check phase doesn't care about this at all. The only thing that cares about this is dependency resolution and the final step of "compile_main". And it could be argued that it is not something that dependency resolution should care about (e.g. cargo allows a lib.rs with a main function and clippy warns you it is unused). Additionally, the only heuristic for determining this difference was the name lib.rs vs main.rs which gets really clunky when we start allowing custom paths (such as how cargo allows it).

That makes sense, but I think we want to be explicit in defining a crate to be a library or a binary, then if there is a main function in a library crate, "clippy" can give a warning. Instead of it being inferred via the main function

@phated
Copy link
Contributor Author

phated commented Jul 7, 2023

That makes sense, but I think we want to be explicit in defining a crate to be a library or a binary, then if there is a main function in a library crate, "clippy" can give a warning. Instead of it being inferred via the main function

If you want to give a clippy-style warning, this should just be a "nargo thing" not a "noirc thing" (or "frontend thing"). The frontend should have no concept of the crate type

@phated phated changed the title feat: Remove the concept of CrateType chore: Remove the concept of CrateType Jul 7, 2023
@phated phated force-pushed the phated/remove-crate-type branch from 60db9c6 to 22bda3e Compare July 7, 2023 19:35
@TomAFrench TomAFrench changed the title chore: Remove the concept of CrateType chore: Remove the concept of CrateType Jul 10, 2023
@TomAFrench
Copy link
Member

Looks like this has been blocked by #1913. We'll need to revert/rework some of the changes in this PR to be able to progress on this.

@phated phated marked this pull request as draft July 18, 2023 22:43
@phated
Copy link
Contributor Author

phated commented Jul 18, 2023

Converting to draft, as my rebase removes some of the premature-merge workspace logic

@phated phated force-pushed the phated/remove-crate-type branch from 22bda3e to ebd55ba Compare July 18, 2023 22:44
@phated
Copy link
Contributor Author

phated commented Aug 3, 2023

This is being replaced by #2134

@phated phated closed this Aug 3, 2023
@TomAFrench TomAFrench deleted the phated/remove-crate-type branch November 20, 2024 12:01
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.

3 participants