Disallow comments without assertions in binding assertions input files

Also, count the number of skipped assertions (due to not fulfilling the version requirements)
NomicFoundation · Aug 16, 2024 · 635facd · 635facd
1 parent 2177bf2
commit 635facd
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 12 deletions.
diff --git a/crates/solidity/outputs/cargo/tests/src/bindings_assertions/assertions.rs b/crates/solidity/outputs/cargo/tests/src/bindings_assertions/assertions.rs
@@ -21,6 +21,9 @@ pub enum AssertionError {
     #[error("Duplicate assertion definition {0}")]
     DuplicateDefinition(String),
 
+    #[error("No assertions found in line {0}")]
+    NoAssertionsFound(usize),
+
     #[error("Failed {failed} of {total} bindings assertions:\n{errors:#?}")]
     FailedAssertions {
         failed: usize,
@@ -122,7 +125,7 @@ impl<'a> fmt::Display for DisplayCursor<'a> {
 }
 
 /// Collects bindings assertions in comments in the parsed source code
-/// accessible through the given cursor. The definitionns take the following form:
+/// accessible through the given cursor. The definitions take the following form:
 ///
 ///   uint x;
 ///   //   ^def:1
@@ -149,12 +152,16 @@ impl<'a> fmt::Display for DisplayCursor<'a> {
 ///   //  ^ref:2
 ///   //<ref:1
 ///
+/// All single line comments should contain an assertion. It is considered an
+/// error if they don't.
+///
 pub fn collect_assertions_into<'a>(
     assertions: &mut Assertions<'a>,
     mut cursor: Cursor,
     file: &'a str,
     version: &Version,
-) -> Result<(), AssertionError> {
+) -> Result<usize, AssertionError> {
+    let mut skipped = 0;
     loop {
         if cursor
             .node()
@@ -167,7 +174,13 @@ pub fn collect_assertions_into<'a>(
                 Some(Assertion::Reference(assertion)) => {
                     assertions.insert_reference_assertion(assertion);
                 }
-                None => (),
+                Some(Assertion::Skipped) => {
+                    skipped += 1;
+                }
+                None => {
+                    let line = cursor.text_offset().line + 1;
+                    return Err(AssertionError::NoAssertionsFound(line));
+                }
             }
         }
 
@@ -176,13 +189,14 @@ pub fn collect_assertions_into<'a>(
         }
     }
 
-    Ok(())
+    Ok(skipped)
 }
 
 #[derive(Clone, Debug, PartialEq)]
 enum Assertion<'a> {
     Definition(DefinitionAssertion<'a>),
     Reference(ReferenceAssertion<'a>),
+    Skipped,
 }
 
 static ASSERTION_REGEX: Lazy<Regex> = Lazy::new(|| {
@@ -253,7 +267,7 @@ fn find_assertion_in_comment<'a>(
         // Assertion target may not be parseable with the current version
         if let Some(version_req) = version_req {
             if !version_req.matches(version) {
-                return Ok(None);
+                return Ok(Some(Assertion::Skipped));
             }
         }
         Err(AssertionError::InvalidAssertion(

diff --git a/crates/solidity/outputs/cargo/tests/src/bindings_assertions/runner.rs b/crates/solidity/outputs/cargo/tests/src/bindings_assertions/runner.rs
@@ -33,6 +33,7 @@ fn check_assertions_with_version(version: &Version, contents: &str) -> Result<()
     let mut bindings =
         bindings::create_with_resolver(version.clone(), Arc::new(TestsPathResolver {}));
     let mut assertions = Assertions::new();
+    let mut skipped = 0;
 
     let parts = split_multi_file(contents);
 
@@ -50,7 +51,7 @@ fn check_assertions_with_version(version: &Version, contents: &str) -> Result<()
         }
 
         bindings.add_file(file_path, parse_output.create_tree_cursor());
-        collect_assertions_into(
+        skipped += collect_assertions_into(
             &mut assertions,
             parse_output.create_tree_cursor(),
             file_path,
@@ -63,7 +64,7 @@ fn check_assertions_with_version(version: &Version, contents: &str) -> Result<()
     match result {
         Ok(count) => {
             assert!(count > 0, "No assertions found with version {version}");
-            println!("Version {version}, {count} assertions OK");
+            println!("Version {version}, {count} assertions OK, {skipped} skipped");
         }
         Err(err) => {
             panic!("Failed bindings assertions in version {version}:\n{err}");

diff --git a/crates/solidity/testing/snapshots/bindings_assertions/contracts/visibility.sol b/crates/solidity/testing/snapshots/bindings_assertions/contracts/visibility.sol
@@ -25,15 +25,13 @@ contract Second {
         //     ^ref:5
     }
     function get_first_choice() public returns (First.Choice) {
-        // Cannot access a member function through the contract type
         return First.get_choice();
         //     ^ref:1
-        //           ^ref:!
+        //           ^ref:!  -- cannot access a member function through the contract type
     }
     function other_choice() public returns (First.Choice) {
-        // Cannot access a state variable in another contract
         return First.choice;
         //     ^ref:1
-        //           ^ref:!
+        //           ^ref:!  -- cannot access a state variable in another contract
     }
 }
diff --git a/crates/solidity/testing/snapshots/bindings_assertions/scoping/hoisting_scopes.sol b/crates/solidity/testing/snapshots/bindings_assertions/scoping/hoisting_scopes.sol
@@ -1,4 +1,3 @@
-// Before 0.5.0 Solidity hoists all variable definitions
 contract Foo {
   function bar() returns (int x) {
     //                        ^def:1