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

Support native stdlib #201

Merged
merged 37 commits into from
Oct 28, 2024
Merged

Support native stdlib #201

merged 37 commits into from
Oct 28, 2024

Conversation

katat
Copy link
Collaborator

@katat katat commented Oct 11, 2024

This PR adds initial native stdlib: comparator and bits, enabling use cases such as bit decomposition and compare values. (these are the two modules copied from the pending PR #197)

Besides, it includes a new download method for stdlib. Currently, instead of treating these stdlib as packages like the user/repo modules, it uses git clone to fetch the branch of a target repo. Then individually processes the .no files as different modules like std/bits and std/comparator.

Alternatively to relying on git command, stdlib could be downloadeded from the latest released version on github.

TODO:

  • Change the hardcoded repo for downloading stdlib(currently it uses my repo for testing)
  • Decide if downloading from github release page is preferred

@katat
Copy link
Collaborator Author

katat commented Oct 18, 2024

To avoid making this PR too big to review, there are 2 separate PRs depending on this one for the updates:

Copy link
Contributor

@mimoo mimoo left a comment

Choose a reason for hiding this comment

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

so a few things:

  • we should definitely move to using this repo instead of your own repo ^^
  • we should also find a way to version the stdlib and allow updates, but maybe let's pun on that for now in the spirit of moving fast (this is related to versioning and release in general)
  • there's a number of changes that seem unrelated to this PR

src/tests/mod.rs Outdated Show resolved Hide resolved
use std::bits;

struct Uint8 {
// todo: maybe add a const attribute to Field to forbid reassignment
Copy link
Contributor

Choose a reason for hiding this comment

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

imo if we do this we should do it the other way around: mark fields that can be mutated without going through the API (through the pub keyword like in rust)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yeah, using pub keyword makes more sense, as it is about whether it is accessible publicly.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw generally in security you want to have a whitelist rather than a blacklist

return;
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we care if fields of a struct are const? I don't think this is a good idea (for example https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f5142c25ee550e0a982421e8aa7ef876 won't compile)

src/stdlib/native/int.no Outdated Show resolved Hide resolved
let bit_num = unsafe nth_bit(value, index);

// constrain the bit_num to be 0 or 1
assert_eq(bit_num * (bit_num - 1), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to be able to write Bool::new(bit_num) or Bool(bit_num) instead

but not sure how to do that ^^ builtin? a bool.no library? it's hard because the Bool type is a builtin type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/stdlib/native/bits.no Outdated Show resolved Hide resolved
src/stdlib/native/bits.no Outdated Show resolved Hide resolved
src/stdlib/mod.rs Outdated Show resolved Hide resolved
pub fn download_stdlib() -> Result<()> {
// Hardcoded repository details and target branch
let repo_owner = "katat";
let repo_name = "noname";
Copy link
Contributor

Choose a reason for hiding this comment

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

is that a temporary solution :D

one thing we could do is have a github workflow to remove everything but the lib and push that on a stdlib branch or something. This way we can just git clone this repo with only the latest commit on that branch. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

otherwise it feels a bit wasteful to import all of noname?

ALTHOUGH, it would be cool if, when people click on "go to definition" we could also point to the source code of noname

so it might be an upside to have the whole noname code pulled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's create a release branch for now? so we can decide which version of main branch to be merged into the release branch for auto-download

let prefix_stdlib = Path::new(path_prefix);
let code = std::fs::read_to_string(prefix_stdlib.join(format!("{lib}.no"))).unwrap();
node_id =
typecheck_next_file(tast, Some(module), sources, lib.to_string(), code, 0).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

we should really have a pass that lists the dependencies use, with this approach we're processing a bunch of stdlibs that might not even be used :o

Copy link
Contributor

Choose a reason for hiding this comment

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

cf #29

@katat
Copy link
Collaborator Author

katat commented Oct 24, 2024

we should definitely move to using this repo instead of your own repo ^^

How about we create a release branch in this repo for the auto download? This release branch allow us to control the latest version of stdlib.

we should also find a way to version the stdlib and allow updates, but maybe let's pun on that for now in the spirit of moving fast (this is related to versioning and release in general)

Another way to download code other than release branch, is to make releases in github release page and have the code to automatically download from the release page.

Beside, we would need to provide a way for the users to configure their noname packages to decide which versions of stdlib to download.

@katat katat mentioned this pull request Oct 24, 2024
@katat
Copy link
Collaborator Author

katat commented Oct 24, 2024

there's a number of changes that seem unrelated to this PR

They are mostly about propagating the const value from struct fields to propagate self.bit_len as the constant argument for a generic function call. Maybe I should revert them until we got generic struct.

* add uint16/32/64
* allow [0][0]
@katat
Copy link
Collaborator Author

katat commented Oct 24, 2024

The CI error is due to the outdate stdlib code from my repo.

For the release branch (if we decided to use that branch for downloading stdlib), we would need to:

  • merge this branch into the main
  • init the release branch from this branch

Then it would resolve the CI error for the subsequent PRs.

@katat katat changed the base branch from feat/hint-builtin-wir to main October 24, 2024 10:47
@mimoo
Copy link
Contributor

mimoo commented Oct 24, 2024

I think it's fine not to use a release branch and use whatever is the latest. Then we can add stdlib to the list of things we need to version/release correctly once we decide to tackle that :o

for the const fields in struct I think it can have its uses, but I'm a bit worried about edge cases (what if we use a const at some point, but then mutate the struct with a non-const, etc.

@@ -9,6 +9,10 @@ fi
DIR_PATH=$1
CURVE=$2

# Init stdlib in .noname/release/src/stdlib instead of downloading
echo "Overriding stdlib in .noname/release/src/stdlib..."
mkdir -p ~/.noname/release/src/stdlib/ && cp -r /app/noname/src/stdlib/* ~/.noname/release/src/stdlib/
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should do this manually BTW, we should let noname do it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is because the CI test wants to test against the latest code, including the stdlib. To ensure the latest code, here overrides the default downloaded version with the code from the PR.

katat added 2 commits October 28, 2024 14:32
* add multiplexer stdlib
* add mimc stdlib
@katat katat merged commit bde1e37 into main Oct 28, 2024
1 check passed
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.

2 participants