Skip to content

Add check for new_no_model methods with model types#133

Closed
GideonBature wants to merge 1 commit intorust-bitcoin:masterfrom
GideonBature:verify
Closed

Add check for new_no_model methods with model types#133
GideonBature wants to merge 1 commit intorust-bitcoin:masterfrom
GideonBature:verify

Conversation

@GideonBature
Copy link
Contributor

This PR adds a verification step to ensure that methods marked as new_no_model don't have corresponding model types. If a model type is found for such methods, the verification fails with a clear error message suggesting to change the method marking to new_modelled instead.

Changes

  • Added a new verification function verify_no_model_methods that checks for this inconsistency
  • Integrated this check into the main verification flow
  • Added a helper function type_physically_exists to check if a model type exists regardless of whether it's required
  • Fixed an inconsistency found during testing of the newly added check: changed getblockfilter for v19 from new_no_model to new_modelled as it actually has a model type

This helps prevent inconsistencies between method type annotations and the actual model implementation. When methods evolve across versions and begin to require model types, this check will catch cases where we forgot to update the method annotation from new_no_model to new_modelled.

Closes #70

@tcharding
Copy link
Member

Mad, you found a bug in verify. We can fix it in a much more simple way however. This diff fixes the bug but does not fix the new warnings from verify - want to use this?

diff --git a/verify/src/main.rs b/verify/src/main.rs
index 5ab4de8..8015ae3 100644
--- a/verify/src/main.rs
+++ b/verify/src/main.rs
@@ -166,2 +166,6 @@ fn check_types_exist_if_required(version: Version, method_name: &str) -> Result<
         }
+    } else {
+        if model::type_exists(version, method_name)? {
+            eprintln!("found model type when non expected: {}", output_method(out));
+        }
     }
diff --git a/verify/src/model.rs b/verify/src/model.rs
index fc753b2..7080ef3 100644
--- a/verify/src/model.rs
+++ b/verify/src/model.rs
@@ -42,5 +42,3 @@ pub fn type_exists(version: Version, method_name: &str) -> Result<bool> {
     if let Some(Return::Type(s)) = method.ret {
-        if method.requires_model {
-            return crate::grep_for_string(&path(), s);
-        }
+        return crate::grep_for_re_export(&path(), s);
     }

@GideonBature
Copy link
Contributor Author

Mad, you found a bug in verify. We can fix it in a much more simple way however. This diff fixes the bug but does not fix the new warnings from verify - want to use this?

diff --git a/verify/src/main.rs b/verify/src/main.rs
index 5ab4de8..8015ae3 100644
--- a/verify/src/main.rs
+++ b/verify/src/main.rs
@@ -166,2 +166,6 @@ fn check_types_exist_if_required(version: Version, method_name: &str) -> Result<
         }
+    } else {
+        if model::type_exists(version, method_name)? {
+            eprintln!("found model type when non expected: {}", output_method(out));
+        }
     }
diff --git a/verify/src/model.rs b/verify/src/model.rs
index fc753b2..7080ef3 100644
--- a/verify/src/model.rs
+++ b/verify/src/model.rs
@@ -42,5 +42,3 @@ pub fn type_exists(version: Version, method_name: &str) -> Result<bool> {
     if let Some(Return::Type(s)) = method.ret {
-        if method.requires_model {
-            return crate::grep_for_string(&path(), s);
-        }
+        return crate::grep_for_re_export(&path(), s);
     }

Thank you for this :).

@tcharding
Copy link
Member

What is the status of this PR mate?

@GideonBature
Copy link
Contributor Author

What is the status of this PR mate?

I have used the changes you recommend, tested it and it's working quite well.

@tcharding
Copy link
Member

I expected to see only the changes I suggested and not all the original code you wrote. Was there some reason I'm missing that you kept all the additional code?

@tcharding
Copy link
Member

I went back to double check that my comment above was correct and in order to do so I had to re-code my solution. So I just pushed #139.

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.

verify: Check no model

2 participants