Implemented syn::Error::combine for ItemImplInfo and ItemTraitInfo #1065

jaswinder6991 · 2023-08-08T07:10:17Z

This is wip PR which addresses #1006
This initial commit it to get feedback on how to proceed further with this.

What is the best way to test this other than running run_tests.sh
The issue mentioned there could be potentially more implementations in info_extractor. I could not find more, can someone more experienced confirm this?
What else is needed to conclude the PR?

uint · 2023-08-09T15:55:36Z

What you have is a good start and exactly what we want here!

What is the best way to test this other than running run_tests.sh

Good question. Take a look at compilation_tests in near-sdk. all.rs collects all tests. Each of the remaining files is one test.

trybuild is used here to compile source files and test the output of the compiler (pass vs fail with a specific message). See the trybuild repo for details.

It should be possible to add a test that's supposed to fail compilation and produce multiple errors, and have trybuild verify the output matches what's expected.

To run just the compilation tests, try this inside the near-sdk directory:

cargo test --features=unstable compilation_tests

The issue mentioned there could be potentially more implementations in info_extractor. I could not find more, can someone more experienced confirm this?

Yeah. I'd review all functions in the info_extractor directory that return Result<_, syn::Error>. It's a bit of a chore, I know.

If such a function returns errors at multiple points, I'd attempt to collect and combine them like you've done for ItemImplInfo::new. It might not always be possible and that's fine. Sometimes a later error check might rely on a previous check passing for some data to be available.

For example, here's one more fn I found. At a glance, it has 4 points where it might return an error and they look like they can all be combined.

What else is needed to conclude the PR?

I think you've figured it all out. Implementation + tests is what we need. Then if all checks pass and there's an approving review, we're good to merge.

jaswinder6991 · 2023-08-17T07:37:35Z

Hi @uint, I started working on the arg_info.rs function that you mentioned. I could use some advice here:

I am defining dummy values like below in the function. Is this an expected practice for the sdk? Do you have some other suggestion.

near-sdk-rs/near-sdk-macros/src/core_impl/info_extractor/arg_info.rs

Lines 58 to 67 in 9b41078

    
               errors.push(Error::new( 
        
                   original.span(), 
        
                   "Only identity patterns are supported in function arguments.", 
        
               )); 
        
               //Providing dummy variables. 
        
               //Their value won't have any effect as they are not used later in the code except return. 
        
               pat_reference = None; 
        
               pat_mutability = None; 
        
               Ident::new("DUMMY", Span::call_site()) 
        
           }

For catching sanitisation errors, if I use a dummy value for sanitize_self of there is an error, wouldn't that cause problems later on?
Also in extracting reference, mutability and type. I could not find suitable dummy variables to be used if there is an error. Mostly because of syn::Type.
Should I pursue this more or is it okay to leave as is.

near-sdk-rs/near-sdk-macros/src/core_impl/info_extractor/arg_info.rs

Lines 70 to 73 in 9b41078

    
           let sanitize_self = utils::sanitize_self(&original.ty, source_type)?; 
        
           *original.ty.as_mut() = sanitize_self.ty; 
        
           let (reference, mutability, ty) = 
        
               utils::extract_ref_mut(original.ty.as_ref(), original.span())?;

I am writing compilation test which would fail and check if all combined errors are returned.

uint · 2023-08-17T14:26:32Z

I am defining dummy values like below in the function. Is this an expected practice for the sdk? Do you have some other suggestion.

The code is correct, but at this point the whole new constructor is going to grow into a small ball of spaghetti. It's probably a good time to refactor this a bit.

For catching sanitisation errors, if I use a dummy value for sanitize_self of there is an error, wouldn't that cause problems later on?

I looked at why sanitize_self and extract_ref_mut might fail and it seems they'd fail for the exact same reasons (upon encountering types other than expected) with the same error messages. I don't think we need to combine these two.

My approach here would be to first collect all the data behind Result variables, and then match against all these result/error types at once.

The linked snippets are broad strokes, not a complete solution. Hope this helps!

…ferent outcomes later, wrote compilation test for combined errors for arg_info too.

jaswinder6991 · 2023-08-23T18:48:45Z

Hi @uint, thanks for you suggestions and the code snippets, They were really helpful. In my latest commit 5eca06c, I did a refactor accordingly. Collecting all results and matching them against possible Ok or Error outcomes and returning the errors if any.
I also wrote a compilation test to check whether combined errors are returned.
Let me know if you have any further suggestions.

uint · 2023-08-28T14:39:21Z

near-sdk-macros/src/core_impl/info_extractor/arg_info.rs

+                original: original.clone(),
+            }),
+            _ => {
+                more_errors.extend(pat_info.err());


Clever and succinct!

uint · 2023-08-28T15:19:22Z

near-sdk/compilation_tests/invalid_arg_pat.stderr

   |
-12 |     pub fn insert(&mut self, (a, b): (u8, u32)) {}
-   |                              ^^^^^^
+16 |     pub fn faulty_method1(&mut self, #[serializer(SomeNonExistentSerializer)] (a, b): (u8, u32)) {}


Alright, this is a possible regression. Previously, the tuple was highlighted here. Now it's the the # of the attribute.

uint

It's looking good overall. Just small things, particularly around what code tokens get highlighted in those error messages.

uint · 2023-08-28T15:19:40Z

near-sdk/compilation_tests/invalid_arg_pat.stderr

   |
-12 |     pub fn insert(&mut self, (a, b): (u8, u32)) {}
-   |                              ^^^^^^
+16 |     pub fn faulty_method1(&mut self, #[serializer(SomeNonExistentSerializer)] (a, b): (u8, u32)) {}


Alright, this is a possible regression. Previously, (a, b) was highlighted here. Now it's the the # of the attribute.

uint · 2023-08-28T15:22:25Z

near-sdk/compilation_tests/invalid_arg_pat.stderr

+error: Unsupported contract API type.
+  --> compilation_tests/invalid_arg_pat.rs:15:37
+   |
+15 |     pub fn faulty_method(&mut self, #[serializer(SomeNonExistentSerializer)] _a: *mut u32) {}


Ideally, this would highlight SomeNonExistentSerializer. The error would be much more helpful then.

uint · 2023-08-28T15:23:13Z

near-sdk/compilation_tests/invalid_arg_pat.rs

@@ -1,5 +1,8 @@
-//! Method with non-deserializable argument type.
-
+// //! Method with non-deserializable argument type.


Suggested change

// //! Method with non-deserializable argument type.

//! Method with non-deserializable argument type.

jaswinder6991 · 2023-08-29T17:52:47Z

@uint Thanks for pointing out the error span problems. I actually ended up reading more about it and used syn::new_spanned which gives better error spans and now the errors are more accurate. Maybe we should use it across the whole project. Let me know if you have more suggestions.

…into info_extractor-error_combine

frol · 2023-09-05T11:40:21Z

@jaswinder6991 Please, make sure CI passes and mark this PR ready for review once you believe it is ready (make sure you do a review pass yourself before pinging maintainers)

frol · 2023-10-19T11:23:25Z

This PR was superseded by #1097

wip: implemented Error::combine for ItemImplInfo and ItemTraitInfo

991e3b6

jaswinder6991 mentioned this pull request Aug 8, 2023

Refactor info_extractor to accumulate compilation errors #1006

Closed

wip: implementing error combine for info_extractor argo_info

9b41078

jaswinder6991 added 2 commits August 23, 2023 23:59

refactored arg_info to collect all results and match them against dif…

5eca06c

…ferent outcomes later, wrote compilation test for combined errors for arg_info too.

fixed invalid_arg_pat compilation test's stderr file.

6ff9622

uint reviewed Aug 28, 2023

View reviewed changes

uint suggested changes Aug 28, 2023

View reviewed changes

syn::new_spanned added for more precise error spans.

011d2eb

Merge branch 'master' of https://github.com/jaswinder6991/near-sdk-rs …

ee22d39

…into info_extractor-error_combine

frol closed this Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented syn::Error::combine for ItemImplInfo and ItemTraitInfo #1065

Implemented syn::Error::combine for ItemImplInfo and ItemTraitInfo #1065

jaswinder6991 commented Aug 8, 2023

uint commented Aug 9, 2023

jaswinder6991 commented Aug 17, 2023

uint commented Aug 17, 2023

jaswinder6991 commented Aug 23, 2023

uint Aug 28, 2023 •

edited

Loading

uint Aug 28, 2023

uint left a comment

uint Aug 28, 2023

uint Aug 28, 2023

uint Aug 28, 2023

jaswinder6991 commented Aug 29, 2023

frol commented Sep 5, 2023

frol commented Oct 19, 2023

		@@ -1,5 +1,8 @@
		//! Method with non-deserializable argument type.

		// //! Method with non-deserializable argument type.

Implemented syn::Error::combine for ItemImplInfo and ItemTraitInfo #1065

Implemented syn::Error::combine for ItemImplInfo and ItemTraitInfo #1065

Conversation

jaswinder6991 commented Aug 8, 2023

uint commented Aug 9, 2023

jaswinder6991 commented Aug 17, 2023

uint commented Aug 17, 2023

jaswinder6991 commented Aug 23, 2023

uint Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

uint Aug 28, 2023

Choose a reason for hiding this comment

uint left a comment

Choose a reason for hiding this comment

uint Aug 28, 2023

Choose a reason for hiding this comment

uint Aug 28, 2023

Choose a reason for hiding this comment

uint Aug 28, 2023

Choose a reason for hiding this comment

jaswinder6991 commented Aug 29, 2023

frol commented Sep 5, 2023

frol commented Oct 19, 2023

uint Aug 28, 2023 •

edited

Loading