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

Feature: make test_utils macros public #1599

Closed

Conversation

odesenfans
Copy link
Contributor

@odesenfans odesenfans commented Feb 5, 2024

This PR makes all macros in the test_utils module public to allow for easier testing of hints (and other features) living in other crates/repositories.

Fixes #1595.

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.
    • CHANGELOG has been updated.

vm/src/utils.rs Outdated
@@ -76,7 +76,7 @@ pub mod test_utils {
Into::<num_bigint::BigInt>::into($val)
};
}
pub(crate) use bigint;
pub use bigint;
Copy link
Member

Choose a reason for hiding this comment

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

This is not really vm-related, so I don't think it should be included in the vm's public api. Moreover, we may choose to remove the usage of BigInt from the VM altogether and remove this macro too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I made it public out of simplicity because keeping it pub(crate) in a now public module induces a few unused import warnings. I'll find a solution.

vm/src/utils.rs Outdated
@@ -87,15 +87,15 @@ pub mod test_utils {
num_bigint::BigInt::parse_bytes($val.as_bytes(), $opt).expect("Couldn't parse bytes")
};
}
pub(crate) use bigint_str;
pub use bigint_str;
Copy link
Member

Choose a reason for hiding this comment

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

This is not really vm-related, so I don't think it should be included in the vm's public api. Moreover, we may choose to remove the usage of BigInt from the VM altogether and remove this macro too.

vm/src/utils.rs Outdated

#[macro_export]
macro_rules! biguint {
($val : expr) => {
Into::<num_bigint::BigUint>::into($val as u128)
};
}
pub(crate) use biguint;
pub use biguint;
Copy link
Member

Choose a reason for hiding this comment

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

This is not really vm-related, so I don't think it should be included in the vm's public api. Moreover, we may choose to remove the usage of BigUint from the VM altogether and remove this macro too.

@@ -106,7 +106,7 @@ pub mod test_utils {
num_bigint::BigUint::parse_bytes($val.as_bytes(), $opt).expect("Couldn't parse bytes")
};
}
pub(crate) use biguint_str;
pub use biguint_str;
Copy link
Member

Choose a reason for hiding this comment

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

This is not really vm-related, so I don't think it should be included in the vm's public api. Moreover, we may choose to remove the usage of BigUint from the VM altogether and remove this macro too.

@@ -155,8 +158,9 @@ pub mod test_utils {
}
};
}
pub(crate) use memory_from_memory;
pub use memory_from_memory;
Copy link
Member

Choose a reason for hiding this comment

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

This is an auxiliary macro used to implement the memory macro, it shouldn't be made public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These macros need to be public to make the memory and segments macros usable. Users need to import them as well.

@@ -189,17 +193,19 @@ pub mod test_utils {
}
};
}
pub(crate) use memory_inner;
pub use memory_inner;
Copy link
Member

Choose a reason for hiding this comment

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

This is an auxiliary macro used to implement the memory macro, it shouldn't be made public

vm/src/utils.rs Outdated

pub(crate) use crate::stdlib::{collections::BTreeMap, sync::Arc};
pub use crate::stdlib::{collections::BTreeMap, sync::Arc};
Copy link
Member

Choose a reason for hiding this comment

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

This is internal to the VM, we shouldn't make it public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only solution I found for this is to split the import in two and

    #[allow(unused_imports)]
    pub(crate) use crate::stdlib::collections::BTreeMap;
    pub(crate) use crate::stdlib::sync::Arc;

Tell me if you have a better solution.

vm/src/utils.rs Outdated
pub(crate) use crate::types::program::HintsCollection;
pub(crate) use crate::types::program::Program;
pub use crate::types::program::Program;
Copy link
Member

Choose a reason for hiding this comment

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

Program is already public, we should use the full path in the macro rather than re-exporting it here

@odesenfans
Copy link
Contributor Author

This PR should be mergeable. Most of the macros need to be public as they are used in public macros. I addressed all the comments that could be.

@@ -92,8 +92,8 @@ impl AddressSet {
}

pub struct Memory {
pub(crate) data: Vec<Vec<Option<MemoryCell>>>,
Copy link
Member

Choose a reason for hiding this comment

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

Why did you made these 2 fields public? they are private to make it easier to refactor the representation and improve it. There are many methods for retrieving and inspecting the Memory and pretty-printing it (Display instance).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same, some macros access these fields directly so they must be public to use the macros out of tree. I'll try to think of another solution.

@@ -15,7 +15,7 @@ use crate::{
pub struct MemorySegmentManager {
pub segment_sizes: HashMap<usize, usize>,
pub segment_used_sizes: Option<Vec<usize>>,
pub(crate) memory: Memory,
pub memory: Memory,
Copy link
Member

Choose a reason for hiding this comment

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

this should not be made public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the macros in test_utils require access to this field. As the goal is to make the macros usable out of tree, the easy solution is to make this field public. I understand the concern though. Any suggestion on how to work around the issue?

Copy link
Member

Choose a reason for hiding this comment

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

Any suggestion on how to work around the issue?

It depends on what you want to do in the tests. Maybe you could just use the insert family of functions.

Copy link
Member

@juanbono juanbono left a comment

Choose a reason for hiding this comment

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

See comments

@fmoletta
Copy link
Member

fmoletta commented Apr 8, 2024

Hello! Making these macros public in the current state involves making a lot of fields internal to the VM public which is not desirable at all. I think we should address this task in a different way. What these macros currently do can be implemented using the vm's public api, so we should re-implement or implement new macros that use the public api instead.

@odesenfans
Copy link
Contributor Author

Ok @fmoletta then let's close this PR, no worries.

@odesenfans odesenfans closed this Apr 10, 2024
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.

Make it easier to test hints implemented out of the cairo-vm repository
4 participants