Is the rustified API actually rusty?

[NinekoTheCat]

whilst trying to use this library in a project of mine I've come across an interesting question, is the rustified API actually rusty?
generally what seems to have happened is a lot of specific 'C' semantics sneaked their way into the crate, for example the Node type has a lot of 'as' functions which not only conflict with rusts as operator but also are equivalent to an if let statement when checking for a specific node type.
the parse function seems to just return a generic Node, even though as far as I'm aware it can only be a specific type of node?

the API will panic if the 'c parser fails to initialize', however that decision should be left up to the caller and an error should be passed instead with the Result union type.

[NinekoTheCat]

consumers like rubyfmt have to sometimes even redefine their own Node type because the API is afraid of owning anything that's returned from C for some reason.
see https://github.com/ryym/rbfmt/blob/c5a2f2bc96733fdcd67283d9c180f87e81fd4ca8/src/parse.rs#L29 for an example of a consumer doing this.

[froydnj]

Just a small note: rubyfmt uses Prism's node types directly: https://github.com/fables-tales/rubyfmt/blob/95210d780317a4920ac6aae9c0a7d5730c0739d1/librubyfmt/src/format_prism.rs

rbfmt is, of course, free to do what it wants.

[NinekoTheCat]

prism/rust/ruby-prism/src/node.rs

Line 134 in 2b87feb

pub fn as_slice(&self) -> &'pr [u8] {

seems to also panic instead of gracefully returning a Result?

[NinekoTheCat]

Arguably ParseResult should be owned because the parser is (assumedly) passing the responsibility of managing its memory onto the caller?

[froydnj]

The ParseResult's lifetime is tied to the lifetime of the input, because the underlying C implementation is holding pointers to the slice. What's wrong with that?

[NinekoTheCat]

The problem is that at least when I tried to use it I had to do a lot of borrower gymnastics to pass anything around.

[NinekoTheCat]

ah the build.rs file autogenerates the bindings.rs library which is why so much strangeness is going around @kddnewton would it be alright if I took a look and maybe changed a bunch of stuff there to make more sense?

[NinekoTheCat]

IE for the rust bindings to be actually rusty we'd need to rewrite the build.rs script such that it actually takes ownership and has proper destructors for all the types it creates.

[froydnj]

Why do any of the types need proper destructors? And why is taking ownership necessary in any way?

[NinekoTheCat]

because I assume the c impl assumes that we are now in charge of whenever or not ParseResult is valid (removing the data it is pointing to)

[froydnj]

I'm not sure I understand what that assumption is getting at.

ParseResult needs a destructor, because it's holding on to things allocated in C:

prism/rust/ruby-prism/src/parse_result/mod.rs

Lines 286 to 293 in 2b87feb

	impl Drop for ParseResult<'_> {
	fn drop(&mut self) {
	unsafe {
	pm_parser_free(self.parser);
	pm_arena_free(self.arena);
	}
	}
	}

which I think agrees with your assumption that ParseResult is in charge of whether the underlying data is valid?

I don't see why anything else needs a destructor. Nodes, for instance, are holding pointers to things allocated in C, but those things are (transitively) owned by ParseResult itself, and nodes are (assuming that we have written all the unsafe code correctly) tied to the lifetime of the ParseResult. So nodes never have to worry about destructing anything when they are destroyed, because a) you can't use a node without holding on to a ParseResult somewhere and b) the ParseResult is handling the destruction of the resources defined in C.

FWIW, having written several non-trivial things with Prism's Rust bindings, I do agree that the API is maybe not as smooth as it could be. But having gotten frustrated with the API once and having tried to rewrite it, I think you will find that to make it smoother (e.g. not having to deal with lifetimes), you would have to make it substantially less efficient or substantially less safe, and neither one is appropriate for Rust bindings. (I could be wrong about the rewrite, though!)

[NinekoTheCat]

@froydnj alright but I'll move a bunch of things that should be macros in the build.rs for the crate into a separate macros crate for it so that it's more clear what's going on with the bindings generation.