Makes IonSchemaElement use borrowed Elements #222

popematt · 2024-10-14T18:12:28Z

Issue #, if available:

Fixes #214
Fixes #137

Description of changes:

Changes IonSchemaElement so that it holds references to Elements rather than owning Elements. Details in PR tour 🗺️ comments.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Makes IonSchemaElement use borrowed Elements

popematt · 2024-10-14T18:25:59Z

ion-schema-tests-runner/src/generator.rs

+            let schema = __new_schema_system().load_schema(schema_id).expect(&format!("Expected to load schema: {}", schema_id));
+            let isl_type = schema.get_type(type_id).expect(&format!("Expected to get type: {}", type_id));
+            let value: ion_rs::Element = ion_rs::Element::read_one(value_ion.as_bytes()).expect(&format!("Expected to be able to read value: {}", value_ion));


🗺️ Drive-by improvement to get better failure messages in the test runner.

I'm not very familiar with this part of the code--it looks like this is a method that only gets run in testing, so performance may not matter. If so, you can disregard.

These expect(&format!(...)) calls will allocate/format a String even if the previous method calls succeed. We should use something like Result::unwrap_or_else(|| panic!("Expected ...")) to sidestep the cost on the happy path.

https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_else

It's only in testing.

popematt · 2024-10-14T18:29:30Z

ion-schema-tests-runner/tests/ion-schema-tests-2-0.rs

        "imports",
        "constraints::ordered_elements",
-        "constraints::precision",


🗺️ It turns out that we don't need to skip the tests for precision any more. However, the fix was in some previous change. All of the other changes in this file are just trying to improve the specificity of the comments.

popematt · 2024-10-14T18:42:23Z

ion-schema/src/lib.rs

@@ -93,125 +92,6 @@ const ISL_2_0_KEYWORDS: [&str; 28] = [
    "valid_values",
 ];

-/// Provide an Ion schema Element which includes all Elements and a document type


🗺️ All of this was moved to ion_schema_element.rs (and then rewritten).

popematt · 2024-10-14T19:34:22Z

ion-schema/src/nfa.rs

@@ -156,8 +155,11 @@ impl NfaEvaluation {
    }

    /// Validates provided ordered elements against referenced [Nfa]
-    pub fn validate_ordered_elements(&mut self, elements: &[Element], type_store: &TypeStore) {
-        let mut elements_iter = elements.iter().peekable();
+    pub fn validate_ordered_elements<'a, T: Iterator<Item = &'a Element>>(


🗺️ The NFA implementation now accepts any iterator over Elements, rather than requiring a slice of Elements or being tied to a particular iterator implementation.

popematt · 2024-10-14T19:39:04Z

ion-schema/src/system.rs

-    // this is just a place holder for document type,
-    // IonSchemaElement::Document(_) type is used to verify the correctness on the validation side
-    "type::{ name: document }",


🗺️ document is an "atomic" type instead of a "derived" type now that we have IonSchemaElementType::Document.

zslayton · 2024-10-15T13:07:59Z

ion-schema-tests-runner/src/generator.rs

+            let schema = __new_schema_system().load_schema(schema_id).expect(&format!("Expected to load schema: {}", schema_id));
+            let isl_type = schema.get_type(type_id).expect(&format!("Expected to get type: {}", type_id));
+            let value: ion_rs::Element = ion_rs::Element::read_one(value_ion.as_bytes()).expect(&format!("Expected to be able to read value: {}", value_ion));


I'm not very familiar with this part of the code--it looks like this is a method that only gets run in testing, so performance may not matter. If so, you can disregard.

These expect(&format!(...)) calls will allocate/format a String even if the previous method calls succeed. We should use something like Result::unwrap_or_else(|| panic!("Expected ...")) to sidestep the cost on the happy path.

https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_else

zslayton · 2024-10-15T13:08:51Z

ion-schema-tests-runner/src/generator.rs

@@ -321,18 +321,15 @@ fn generate_preamble(root_dir_path: &Path) -> TokenStream {

        /// Asserts that a value is or is not valid for a given ISL type.
        fn __assert_value_validity_for_type(value_ion: &str, schema_id: &str, type_id: &str, expect_valid: bool) -> Result<(), String> {


Do you happen to know why we're using a __ prefix rather than, say, making a new submodule to house these functions?

I wanted to make sure that we didn't have any name conflicts with a generated test case, so I added a __ prefix. I could have added a new submodule, but that probably also would have needed something to reduce the likelihood of a name conflict with a generated test function.

zslayton · 2024-10-15T13:33:49Z

ion-schema/src/nfa.rs

-        let mut elements_iter = elements.iter().peekable();
+    pub fn validate_ordered_elements<'a, T: Iterator<Item = &'a Element>>(
+        &mut self,
+        mut elements_iter: Peekable<T>,


TIL that Peekable's impl of peekable() is not specialized to avoid producing a Peekable<Peekable<T>>.

zslayton · 2024-10-15T13:42:01Z

ion-schema/src/nfa.rs

@@ -267,12 +269,12 @@ impl NfaEvaluation {
    // this method iterates through elements to satisfy minimum required occurrences for given transition
    // It will return false if an invalid Ion value is found which doesn't satisfy minimum occurrence requirement for given transition
    // Otherwise it will return true
-    fn evaluate_transition_to_self(
+    fn evaluate_transition_to_self<'a, T: Iterator<Item = &'a Element>>(


Food for thought: you could create a new trait for this to avoid having to spell out the bounds each time.

// Make an empty extension trait called `ElementIter`... trait ElementIter<'a>: Iterator<Item = &'a Element> {} impl<'a, T> ElementIter<'a> for T where T: Iterator<Item = &'a Element> {} // ...and then replace this: fn evaluate_transition_to_self<'a, T: Iterator<Item = &'a Element>> (...) {...} // ...with this: fn evaluate_transition_to_self<'a, T: ElementIter<'a>>(...) {...}

Yeah, I guess I could. TBH, though, I'm hoping to do a fairly complete rewrite the NFA to get better error messages like in amazon-ion/ion-schema-kotlin#228

popematt force-pushed the borrow-elements branch 2 times, most recently from 63cb7fc to 32a6cf5 Compare October 14, 2024 19:04

Makes IonSchemaElement borrow rather than own

ec65db9

Makes IonSchemaElement use borrowed Elements

popematt force-pushed the borrow-elements branch from 32a6cf5 to ec65db9 Compare October 14, 2024 19:37

popematt commented Oct 14, 2024

View reviewed changes

cargo fmt fixes

e0f485b

popematt marked this pull request as ready for review October 14, 2024 19:50

zslayton approved these changes Oct 15, 2024

View reviewed changes

popematt merged commit 4b27d7f into amazon-ion:main Oct 15, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes IonSchemaElement use borrowed Elements #222

Makes IonSchemaElement use borrowed Elements #222

popematt commented Oct 14, 2024

popematt Oct 14, 2024

zslayton Oct 15, 2024

popematt Oct 15, 2024

popematt Oct 14, 2024

popematt Oct 14, 2024

popematt Oct 14, 2024

popematt Oct 14, 2024

zslayton Oct 15, 2024

zslayton Oct 15, 2024

popematt Oct 15, 2024

zslayton Oct 15, 2024

zslayton Oct 15, 2024

popematt Oct 15, 2024

		@@ -321,18 +321,15 @@ fn generate_preamble(root_dir_path: &Path) -> TokenStream {

		/// Asserts that a value is or is not valid for a given ISL type.
		fn __assert_value_validity_for_type(value_ion: &str, schema_id: &str, type_id: &str, expect_valid: bool) -> Result<(), String> {

Makes IonSchemaElement use borrowed Elements #222

Makes IonSchemaElement use borrowed Elements #222

Conversation

popematt commented Oct 14, 2024

Issue #, if available:

Description of changes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment