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

Recognise that CircuitBuilder::namespace can't fail #494

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

matthiasgoergens
Copy link
Collaborator

At least CircuitBuilder::namespace won't fail on its own. But it might fail, if the circus construction function (or so) you are passing to fails.

The new type reflects that.

Extracted from #466 and addressing https://github.com/scroll-tech/ceno/pull/455/files#r1818907561

This will lead to a lot of clean up down the line, but I want to keep this PR small.

@hero78119
Copy link
Collaborator

hero78119 commented Oct 29, 2024

I have some plan to formalized our namespace to make it more organized.
Which means namespace string should meet with some format, or it will return failed
With that, please onhold for attempt to change on namespace api (and all the namespace/debug map related) first.

@matthiasgoergens
Copy link
Collaborator Author

matthiasgoergens commented Oct 29, 2024

Yes, that's why I'm holding off on #435 for now. @hero78119 This PR is literally just recognising that instead of specialising our return type to Result<X, ZKVMError>, we can use just Y and get the old behaviour by setting Y === Result<T, ZKVMError>. The implementation of fn namespace doesn't change at all, and all uses of the old API are still valid here, it's not really a change at all.

How does your plan look like?

I would suggest going for a more functional approach. If you are still interested in suggestions, I can sketch that out a bit more. But I would also be curious to see what you are cooking up.

@matthiasgoergens matthiasgoergens marked this pull request as ready for review October 29, 2024 10:57
Base automatically changed from matthias/name-space to master October 29, 2024 12:27
This was referenced Oct 30, 2024
@matthiasgoergens matthiasgoergens changed the title Recognise that CircuitBuilder::namespace can't fail either Recognise that CircuitBuilder::namespace can't fail Nov 5, 2024
@@ -321,8 +321,8 @@ impl<'a, E: ExtensionField> CircuitBuilder<'a, E> {
pub fn namespace<NR: Into<String>, N: FnOnce() -> NR, T>(
&mut self,
name_fn: N,
cb: impl FnOnce(&mut CircuitBuilder<E>) -> Result<T, ZKVMError>,
) -> Result<T, ZKVMError> {
cb: impl FnOnce(&mut CircuitBuilder<E>) -> T,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lispc Here's a simple realisation: the namespace function as written actually works for any type T, not just for Result.

Realising that, lets us remove plenty of unnecessary OK(..) and .unwrap() later down.

It's useful to do this, because Result types should be used when we actually have something that can go wrong and should be handled; instead of being dealt with via .unwrap() thoughout. (If all the 'error handling' you ever gonna do is via .unwrap(), you might as well panic!() instead of creating an Err in the first place, at least that gives more useful stack traces.)

@matthiasgoergens
Copy link
Collaborator Author

I have some plan to formalized our namespace to make it more organized. Which means namespace string should meet with some format, or it will return failed With that, please onhold for attempt to change on namespace api (and all the namespace/debug map related) first.

Looks like the namespace reorganisation plan isn't happening. At least not anytime soon. Can we please merge this?

@matthiasgoergens
Copy link
Collaborator Author

@hero78119 Any news on the promised refactoring?

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