Validate Codegen Settings #11

tadeohepperle · 2023-12-07T17:57:36Z

This PR adds functionality to check if the provided type substitute and derive settings are appropriate for a given Type Registry. The validation incrementally builds up an error type that is then returned, containing all the type paths for which special settings were defined, but that do not exist in the metadata.
There is also a function similar_type_paths_in_registry to suggest similar type paths that the user might have meant instead.

jsdw · 2023-12-07T18:32:16Z

typegen/src/typegen/error.rs

+impl std::fmt::Display for SettingsValidationError {
+    fn fmt(
+        &self,
+        f: &mut scale_info::prelude::fmt::Formatter<'_>,
+    ) -> scale_info::prelude::fmt::Result {
+        f.write_fmt(format_args!("{:?}", self))
+    }
+}


I wonder if this could be replaced with the thiserror attr on the struct:

#[error = "{:?}"]

But moreover, I think that a proper Display impl should do better than just debug printing :)

jsdw · 2023-12-07T18:34:33Z

typegen/src/typegen/validate_settings.rs

+
+use super::{error::SettingsValidationError, settings::substitutes::TryIntoSynPath};
+
+impl<'a> TypeGenerator<'a> {


It feels sortof weird to me to have a separate impl block in a different place from the type; I like being able to quite easily see all of the methods that something exposes in a single impl block (of it different constraints, a few impl blocks next to eachother) next to the type definition.

If you want to keep the validation stuff in one place, then perhaps it's best to make this a freestanding function like the ones below?

I also wonder how useful this function actually is, since it will check everything (and in Subxt at least, some things don't want to be checked). I can imagine that for Subxt we'd prefer to use the functions below? Do you have a use case for this overall one?

I think you are right, since we want to be very specific about what type paths to check in subxt, maybe this general validation function is not the right approach. :)
About the splitting into different files: It's just my personal style influenced by how you often define extension classes in C# or Dart. But It is probably better to merge it all into one impl block for people reading the code.

typegen/src/typegen/validate_settings.rs

jsdw · 2023-12-07T18:45:02Z

typegen/src/typegen/settings/substitutes.rs

+pub trait TryIntoSynPath {
+    /// Turns the type into a [`syn::Path`]. Returns None for empty paths.
+    fn syn_path(self) -> Option<syn::Path>;
+}


This is cleaner than the PathSegments stuff I think :)

I don't like it so much that we have to define a new trait for this, but due to the orphan rule, we cannot use TryFrom<T> for syn::Path :(

Yeah, I thought about that too and realised that you had no other choice! The other half way option would have been to define an enum Path { SynPath(syn::Path), TypeInfoPath(typeinfo::Path) } and then a From on that, but I think your approach is cleaner since you only care about getting one of those back!

jsdw · 2023-12-07T18:46:43Z

Generally LGTM! Just a couple of small things!

jsdw

Looks good to me; the real test will be how it works in Subxt/subxt-explorer :)

lexnv · 2023-12-08T08:53:36Z

typegen/src/typegen/error.rs

+    pub substitutes_for_unknown_types: Vec<(syn::Path, syn::Path)>,
+}
+
+impl std::fmt::Display for SettingsValidationError {


nit; It would be interesting to see if we could provide alternative types. Since we cannot find type a::b::Ty, we could inspect the type registry for Ty and return all valid paths.

Which for an xcm MultiLocation it would return both v2 and v3 entries, which might make things easier during development. Tho, this could probably be a follow-up

I have a feeling that Subxt won't be able to use the function that produces this error, so it may well be that it's not that useful for us actually, and instead we'll lean on those other validation.rs functions, one of which does provide a list of similar paths

Yeah, the similar_type_paths_in_registry function can do that, but I have not put it into the SettingsValidationError, to not bloat it too much. We should definitely do this though from the subxt macro side.

lexnv · 2023-12-08T08:55:08Z

typegen/src/typegen/mod.rs

+    ///
+    /// # Panics
+    ///
+    /// If no type with the given id found in the type registry.


Since we are returning a result here, would it be dificult to make this method not panic? Same for below?

Good point; in most places we just have an error variant to capture "type ID not found" so it's probably good to do that here too for consistency.

Oh good catch! The docs are out of date, it actually returns an Err(..) if a wrong id was provided.

lexnv · 2023-12-08T09:00:29Z

typegen/src/typegen/validation.rs

+}
+
+/// Returns types in the PortableRegistry that share the identifier with the input path.
+pub fn similar_type_paths_in_registry(


IIUC, this is only used for testing atm, would changing the code a bit make this more informative?

registry_contains_type_path -> Result<(), Vec<syn::Path>>

if the path is in the registry, return Ok(())

when the paths not present, return alternatives with this method

Then we could say if let Err(alternatives) = registry_contains_type_path(..) and extend the error object with those

Or we could keep registry_contains_type_path as it is, and call similar_type_paths_in_registry on false (thinking out loud :D)

Or we could keep registry_contains_type_path as it is, and call similar_type_paths_in_registry on false

I am inclined to do this (as currently done in validate_substitutes_and_derives_against_registry), because it keeps it a bit more decoupled. Ultimately I am not even sure if we will use validate_substitutes_and_derives_against_registry from the subxt side or if we build some separate logic on top of registry_contains_type_path and similar_type_paths_in_registry.

jsdw · 2023-12-08T09:02:07Z

typegen/src/typegen/validation.rs

+}
+
+/// Returns types in the PortableRegistry that share the identifier with the input path.
+pub fn similar_type_paths_in_registry(


This is good for now; eventually we might want to improve it to find paths that are close (to cater for typos) but that would be a reasonable amount of extra work and dependencies

Ah yeah, maybe some Levenshtein distance calculations? But should be too much for now.

lexnv · 2023-12-08T09:04:08Z

Looks solid! Nice job here!
Left a couple of thoughts; mostly removing the panics. We could probably incorporate the suggestion type paths in another PR if it turns out to be too complicated

tadeohepperle added 30 commits November 21, 2023 20:18

change interface

874e795

recursive derives

ca78ad5

added test and comments

4fe27fa

Merge branch 'master' into recursive-derives

8f3af3b

readd test

ce60070

formatting adjustment

390cd15

fix example and unused dependencies

fcfb267

add documentation to everything.

ffa5ea3

fix bugs in generic recursion of container types

8ae1090

clippy fix

83bda09

fix dependencies

66302dc

remove the generated rs code

f7fff3b

make scale_typegen directly accessible

1be27b3

Debug and Clone derives on relevant structs

7ff03ad

fix comments in code

dcdeef5

add compact encoding differentiation

4cf3e7b

fix pipeline error by hiding scale-typegen in description behind flag

361fa5b

add access to type registry

ddda6e0

give middleware access to transformer to resolve types

9163658

nit: fix docs on recurse_policy

619529c

add struct prefix, fix variant space bug

89e4de0

add cargo machete in CI/CD

1798db5

remove #![deny(unused_crate_dependencies)] in description crate

68cbc09

introduce validation, currently broken tests

68e2857

fix syn path issues

f9bad26

add test for type suggestions

f052257

introduce cache hit policy

8904669

fine grained control over cache hit behavior

119bf73

remove should_continue and other things

cfec37d

Merge branch 'recursive-derives' into tadeohepperle/validate-codegen

588b7a9

tadeohepperle added 4 commits December 7, 2023 17:43

remove smallvec

e0a4e68

remove smallvec

47fae43

generic arguments are working much better now

55f6464

Merge branch 'recursive-derives' into tadeohepperle/validate-codegen

ed27633

tadeohepperle changed the base branch from master to recursive-derives December 7, 2023 17:58

fix test

be6186a

jsdw reviewed Dec 7, 2023

View reviewed changes

typegen/src/typegen/validate_settings.rs Outdated Show resolved Hide resolved

jsdw reviewed Dec 7, 2023

View reviewed changes

Base automatically changed from recursive-derives to master December 7, 2023 21:29

tadeohepperle added 3 commits December 8, 2023 08:53

merge

0f283b0

restructure code

2921dcd

validation error display

1233641

jsdw approved these changes Dec 8, 2023

View reviewed changes

lexnv reviewed Dec 8, 2023

View reviewed changes

jsdw reviewed Dec 8, 2023

View reviewed changes

lexnv approved these changes Dec 8, 2023

View reviewed changes

remove panics from docs

08f60d9

tadeohepperle merged commit d742b08 into master Dec 8, 2023
2 checks passed

tadeohepperle deleted the tadeohepperle/validate-codegen branch December 8, 2023 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Codegen Settings #11

Validate Codegen Settings #11

tadeohepperle commented Dec 7, 2023

jsdw Dec 7, 2023

jsdw Dec 7, 2023 •

edited

Loading

tadeohepperle Dec 7, 2023

jsdw Dec 7, 2023

tadeohepperle Dec 7, 2023

jsdw Dec 8, 2023

jsdw commented Dec 7, 2023

jsdw left a comment

lexnv Dec 8, 2023

jsdw Dec 8, 2023

tadeohepperle Dec 8, 2023

lexnv Dec 8, 2023

jsdw Dec 8, 2023 •

edited

Loading

tadeohepperle Dec 8, 2023

lexnv Dec 8, 2023

tadeohepperle Dec 8, 2023

jsdw Dec 8, 2023

tadeohepperle Dec 8, 2023

lexnv commented Dec 8, 2023


		use super::{error::SettingsValidationError, settings::substitutes::TryIntoSynPath};

		impl<'a> TypeGenerator<'a> {

Validate Codegen Settings #11

Validate Codegen Settings #11

Conversation

tadeohepperle commented Dec 7, 2023

Choose a reason for hiding this comment

jsdw Dec 7, 2023 • edited Loading

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

jsdw commented Dec 7, 2023

jsdw left a 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

jsdw Dec 8, 2023 • edited Loading

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

lexnv commented Dec 8, 2023

jsdw Dec 7, 2023 •

edited

Loading

jsdw Dec 8, 2023 •

edited

Loading