Revert context changes on error

[hdwalters]

Fixes bug #489, by using a procedural macro to generate context manager functions, wrapping syntax parser failure. Also pops scope from the scope stack on syntax parser failure.

[KrosFire]

Looks pretty good, but I believe it needs more documentation. It took me a while to understand this. But maybe I'm just stupid XD

[KrosFire]

Suggested change

	fn visit_field(&mut self, field: &'a Field) {
	if field.attrs.iter().any(utils::is_context) {
	if let Some(name) = &field.ident {
	if let Some(segment) = utils::get_type(field) {
	self.functions.push(Self::make_function(name, segment));
	}
	}
	}
	}
	fn visit_field(&mut self, field: &'a Field) {
	if !field.attrs.iter().any(utils::is_context) {
	return;
	}
	if let (Some(name), Some(segment)) = (&field.ident, utils::get_type(field)) {
	self.functions.push(Self::make_function(name, segment));
	}
	}

[hdwalters]

I'm not sure your suggested tuple destructuring makes things any clearer; better to keep unrelated functionality on separate lines.

[KrosFire]

@Ph0enixKM wdyt? Imo it looks clearer, but It's just a preference so vox populi, vox Dei

[Ph0enixKM]

The modified version looks clearer because it favors the never-nesting approach. I like the change that @KrosFire made.

Early return is more verbose. If we had even more nested if..let then this change would highlight the issue even more. The code will keep the indentation level while the original nested code will be a bunch of if's nested inside.

[KrosFire]

Suggested change

	if let Meta::Path(path) = &attr.meta {
	if let Some(segment) = path.segments.last() {
	let ident = segment.ident.to_string();
	if ident == "context" {
	return true;
	}
	}
	}
	false
	if let Meta::Path(path) = &attr.meta {
	return path
	.segments
	.last()
	.map_or(false, |segment| segment.ident == "context");
	}
	false

[hdwalters]

I'm not sure this makes things any clearer.

[Ph0enixKM]

I think that what @KrosFire suggested is slightly more readable because it eliminates the need for conversion .to_string()

[hdwalters]

I've removed the unnecessary .to_string(), but that .map_or() construct seems unnecessarily verbose. I tend to write nested chains of if statements, because it puts each thing being tested on a single line, reducing the conceptual load.

[KrosFire]

LGTM

[hdwalters]

Change was suggested by RustRover. Not sure if it's sensible; perhaps someone who knows more about doc comments than I do could verify?

[Ph0enixKM]

As per rust book:

bare_urls detects links that are not linkified, e.g., in Markdown such as Go to https://example.com/. It suggests wrapping the link with angle brackets: Go to <https://example.com/>. to linkify it. This is the code behind the rustdoc::bare_urls lint.

To be honest I didn't know that before! It's always nice to learn something new

[Ph0enixKM]

I'm thinking whether or not is the whole proc macro approach a bit of an overkill for such a simple use case. Is there something that macro_rules was inappropriate? The proc macro requires to create a whole new repository. Using macro_rules requires a lot less effort to read the macro, because it usually can be expressed in a single file.

I'm saying that because the implementation for the functions isn't that complex and it seems that we could use a simpler macro for this.

macro_rules! with_context {
	(...) => {
		let prev = self.#name.clone();
		self.#name = #name;
		let result = body(self);
		self.#name = prev;
		result
	}

	// ... Maybe some other implementations for `ref` and `fn`
}

And then the usage would be:

with_context!(..., |...| {

})

Anyways... what do you think @hdwalters @KrosFire?

[hdwalters]

I'm thinking whether or not is the whole proc macro approach a bit of an overkill for such a simple use case. Is there something that macro_rules was inappropriate? The proc macro requires to create a whole new repository. Using macro_rules requires a lot less effort to read the macro, because it usually can be expressed in a single file.
...
Anyways... what do you think @hdwalters @KrosFire?

I think the proc_macro_derive approach is more elegant, because you can turn functionality on and off with Rust annotations, which are (by nature) inline with what they're annotating. The problem with your macro_rules! suggestion is that the macro call site is physically separate from the code it references, and can therefore get out of step.

Also, as I understand it, we do not need to create a new Git repository, just a new Cargo.toml file in a subdirectory; running cargo build on the root directory appears to build the macro dependency automatically.

Also, depending on your IDE (RustRover does this) you can show the results of macro expansion by hovering over the relevant item in the source. This certainly helped me write this!

[hdwalters]

Also, as I understand it, we do not need to create a new Git repository, just a new Cargo.toml file in a subdirectory; running cargo build on the root directory appears to build the macro dependency automatically.

Touched one of the subdirectory source files and rebuilt. The dependency was automatically rebuilt:

hwalters@Ghostwheel ~/git/amber (revert-context-changes-on-error) 
$ touch meta/src/lib.rs 
hwalters@Ghostwheel ~/git/amber (revert-context-changes-on-error) 
$ cargo build
   Compiling amber-meta v0.1.0 (/home/hwalters/git/amber/meta)
   Compiling amber v0.3.5-alpha (/home/hwalters/git/amber)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.12s

So the CI server should continue to work; and I don't believe we're pushing the source to https://crates.io/ or anywhere similar, so there shouldn't be any problems there either.

[Ph0enixKM]

The problem with your macro_rules! suggestion is that the macro call site is physically separate from the code it references, and can therefore get out of step.

@hdwalters could you elaborate on that? I still don't understand the point. You can always modify the variable with std::mem::swap. Oh maybe you meant that the fact that the macro implements a method - this will tell the user to use the method of the struct instead of looking for a macro that somehow is meant to be used for it? I agree then. I didn't see it this way. Good call.

[Ph0enixKM]

We could migrate other macros to this proc macro to enhance the experience even more. But that's not the most important thing to be worried about right now.

[Ph0enixKM]

LGTM

[hdwalters]

@hdwalters could you elaborate on that? I still don't understand the point. You can always modify the variable with std::mem::swap. Oh maybe you meant that the fact that the macro implements a method - this will tell the user to use the method of the struct instead of looking for a macro that somehow is meant to be used for it? I agree then. I didn't see it this way. Good call.

Actually I meant that it's better to configure macro behaviour with annotations, because they're right next to the annotated code, so it's more obvious what's available:

#[derive(ContextManager)]
struct Amplifier {
    #[context]
    power: bool,
    ...
}

But your point is valid too; the main thing about procedural macros is that they operate directly on and (in this case) add functions to the abstract syntax tree, exactly as if you had typed them yourself.

[hdwalters]

Declarative macros with macro_rules! are better for things like print!() and vec![], where they're not tied to a particular struct or field.

[hdwalters]

Rust macros are so much cooler than C++ ones!