refactor: disentangle bindgen extractor logic (continued)

[agostbiro]

Hi,

As discussed with @frol I took over #890 and continued it here.

Thanks for all the great work prior @itegulov ! I really appreciate your refactoring after making a small change in related code yesterday.

TODOs

There were the following unresolved questions in #890 :

1. Whether the trait object based polymorphism approach in the visitor can be improved and subsequently whether build can be made more idiomatic.
2. How handles_result and visit_result relate.
3. Whether view functions can be allowed to be private.

Polymorphism

When looking at the trait object based polymorphism approach I noticed that there is some repetition in the trait implementations. I initially tried to resolve this by providing default implementations, but then realized that the combinations of shared code are not amenable to this. Specifically, given the three kinds of methods (call, view, init), there were no trait method with more than two differing implementations, but how the the the three methods are split into two groups varies.

For this reason, I switched from a trait based approach to an enum based approach, where there is just one visitor object (as opposed to three visitor objects for each method kind implementing a trait). The methods of this single visitor object match on an enum to alter their behavior based on an internal tag that determines the method kind that the visitor object represents.

For example I changed

impl BindgenVisitor for InitVisitor {
    fn visit_init_attr(&mut self, _attr: &Attribute, init_attr: &InitAttr) -> syn::Result<()> {
        self.ignores_state = init_attr.ignore_state;
        Ok(())
    }
}

impl BindgenVisitor for CallVisitor {
    fn visit_init_attr(&mut self, attr: &Attribute, _init_attr: &InitAttr) -> syn::Result<()> {
        Err(Error::new(attr.span(), "Call function can't be an init function at the same time."))
    }
}

impl BindgenVisitor for ViewVisitor {
    fn visit_init_attr(&mut self, attr: &Attribute, _init_attr: &InitAttr) -> syn::Result<()> {
        Err(Error::new(attr.span(), "View function can't be an init function at the same time."))
    }
}

to

impl Visitor {
    pub fn visit_init_attr(&mut self, attr: &Attribute, init_attr: &InitAttr) -> syn::Result<()> {
        use VisitorKind::*;

        match self.kind {
            Init => {
                self.parsed_data.ignores_state = init_attr.ignore_state;
                Ok(())
            }
            Call | View => {
                let message =
                    format!("{} function can't be an init function at the same time.", self.kind);
                Err(Error::new(attr.span(), message))
            }
        }
    }
}

I found that this approach significantly reduced the code size and I found it easier to check the rules for a specific attribute as it's all in one function. It also makes it possible for the build method to take self by value which was brought up in the review for #890.

The downside is that all the parsed data is now in a single internal data structure even though not all methods need all fields from it which is not so clean. In contrast, with the trait based approach each method kind visitor had its own parsed data structure holding only the data the variant supports. I don't think the single parsed data structure should cause problems in practice, because a visitor method errors if it's called on a variant that doesn't support it.

I'm curious to hear what you think about this approach! Happy to adjust it. The change is done in f6ab89d.

handles_result and visit_result

A review in #890 asked whether visit_result needs to be called when handles_result is visited.

Visiting handles_result was not part of the visitor yet, so I added a method called visit_handles_result for it in 3e843b0. I later renamed from visit_handles_result to visit_handle_result_attr in 76be952 for consistency.

I noticed a potential for confusion based on the method name so I renamed visit_result to visit_return_type in 83534ed which is more accurate.

When calling visit_return_type there was a potential for error, because it needs to be called after handles_result is set for correct behavior. I resolved this by merging the build step and the visit_return_type step since there is only ever one return type and calling it in build ensures that it's called last.

Private view functions

A reviewer in #890 commented that private view functions shouldn't be allowed. This wasn't implemented in #890 and when I made the change several tests failed, because methods that take &self as argument are considered private view. Example failing test case:

#[private] pub fn method(&self, #[callback_unwrap] x: &mut u64, y: String, #[callback_unwrap] z: Vec<u8>) { }

On a related note, at first I was surprised that methods that take self by value are view, but I guess that it makes sense since they cannot mutate the object. As I haven't found a test case for this I added one to make sure that method(self) isn't payable in 56394ec.

Next steps

1. We need to resolve whether view functions can be allowed to be private.
2. Once this PR is ready to be merged, as suggested by the original author, I'll follow up follow-up with a PR to clean up the code generation side of bindgen the macro which will be helped by this refactoring.

[frol]

@agostbiro I have not reviewed the code yet, but I checked that buildkite CI fails due to Cargo.lock files in examples being not up-to-date (strum dependency was added [it was already in the dependency tree, so it is not adding anything new, but Cargo.lock gets changed]). Please, compile all the examples (run examples/build_all_docker.sh) and commit all the changes.

[agostbiro]

@agostbiro I have not reviewed the code yet, but I checked that buildkite CI fails due to Cargo.lock files in examples being not up-to-date (strum dependency was added [it was already in the dependency tree, so it is not adding anything new, but Cargo.lock gets changed]). Please, compile all the examples (run examples/build_all_docker.sh) and commit all the changes.

Thanks @frol , I committed the updated Cargo.lock files in 6637e59 and the CI passes now.

[itegulov]

Thank you for such clear and self-contained commits; reviewing your PR was a pleasure!

Your approach to polymorphism makes a lot more sense, thank you for looking into it and writing up your thoughts. It's a shame we lose per-kind data structures that only contain the fields each kind actually needs (writing that part is what made me question what can be private/payable etc, which I see as a benefit for code transparency) and I don't know if removing some code duplication was worth losing it, but regardless I don't hold a strong opinion here and happy to go with what is already in the PR.

A reviewer in #890 #890 (comment) that private view functions shouldn't be allowed. This wasn't implemented in #890 and when I made the change several tests failed, because methods that take &self as argument are considered private.

I think you meant that methods that take &self argument are considered view, right? So, as @austinabell mentioned in the original PR, view methods can never be private. That being said, the current near_bindgen API is flawed in that it does not provide clear control of which methods are view or call. There are a couple of ways we could resolve this that we have discussed in the past but never :

1. Make private, payable and other such modifiers automatically mean that this method is call
2. Create #[call] and #[view] modifiers and slowly phase out the old self-based deduction system
3. Just leave everything as is, it has been working so far

In any case, probably out of scope for this PR, just though I would share this here since its related to the work you are doing. I propose we do not prohibit private view functions for now and move this conversation to a separate issue.

[agostbiro]

Thanks @itegulov for your kind review!

I think you meant that methods that take &self argument are considered view, right?

Yes, you're right, I misspoke and meant view there. I created a follow up issue #1026 to discuss private view methods.

[frol]

@agostbiro This PR looks great to me. Only one nit comment is left, but overall once CI is green, I will be happy to merge it (the comment can be addressed in a separate PR)

[agostbiro]

The Buildkite CI failed on the last commit with this error:

thread 'actix-rt\|system:0\|arbiter:4' panicked at 'failed to start listening on server_addr=0.0.0.0:21547 e=Os { code: 98, kind: AddrInUse, message: "Address already in use" }', chain/network/src/peer_manager/peer_manager_actor.rs:250:29
  | stack backtrace:
  | 0: rust_begin_unwind
  | at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/std/src/panicking.rs:575:5
  | 1: core::panicking::panic_fmt
  | at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:64:14
  | 2: near_network::peer_manager::peer_manager_actor::PeerManagerActor::spawn::{{closure}}
  | 3: tokio::runtime::task::raw::poll
  | 4: tokio::task::local::LocalSet::tick
  | 5: tokio::task::local::LocalSet::run_until::{{closure}}
  | note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
  | Error: unable to fulfill the query request
  |  
  | Caused by:
  | error while sending payload: [error sending request for url (http://localhost:24233/): error trying to connect: tcp connect error: Connection refused (os error 111)]
 

The same Buildkite job succeeded on the previous commit and the only difference between the two is just a comment, so I'm thinking this is likely a transient problem. Is there any way we can trigger the job to run again in Buildkite?

[itegulov]

@agostbiro I have re-run the job, should be good now