Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert context changes on error #508

Merged
merged 13 commits into from
Oct 18, 2024
10 changes: 10 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 10 additions & 6 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,22 @@ rust-version = "1.79"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
heraclitus-compiler = "1.8.0"
similar-string = "1.4.2"
amber-meta = { path = "meta" }
hdwalters marked this conversation as resolved.
Show resolved Hide resolved
chrono = "0.4.38"
clap = { version = "4.4.18", features = ["derive"] }
colored = "2.0.0"
heraclitus-compiler = "1.8.0"
include_dir = "0.7.4"
itertools = "0.13.0"
clap = { version = "4.4.18", features = ["derive"] }
chrono = "0.4.38"
similar-string = "1.4.2"
test-generator = "0.3.1"
include_dir = "0.7.4"

# test dependencies
[dev-dependencies]
tiny_http = "0.12.0"
assert_cmd = "2.0.14"
predicates = "3.1.0"
tempfile = "3.10.1"
tiny_http = "0.12.0"

[profile.release]
strip = true
Expand All @@ -37,6 +38,9 @@ lto = "thin"
[profile.test]
opt-level = 3

[workspace]
members = ["meta"]

# Config for 'cargo dist'
[workspace.metadata.dist]
# The preferred cargo-dist version to use in CI (Cargo.toml SemVer syntax)
Expand Down
12 changes: 12 additions & 0 deletions meta/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[package]
name = "amber-meta"
version = "0.1.0"
edition = "2021"

[lib]
proc-macro = true

[dependencies]
proc-macro2 = "1.0.86"
quote = "1.0.36"
syn = { version = "2.0.68", features = ["visit"] }
48 changes: 48 additions & 0 deletions meta/src/helper.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
use crate::utils;
use proc_macro2::TokenStream as TokenStream2;
use quote::quote;
use syn::visit::Visit;
use syn::{Field, Ident, PathSegment};

pub struct HelperVisitor {
name: Ident,
functions: Vec<TokenStream2>,
}

impl HelperVisitor {
pub fn new(name: &Ident) -> Self {
Self {
name: name.clone(),
functions: Vec::new(),
}
}

fn make_function(name: &Ident, segment: &PathSegment) -> TokenStream2 {
let concat = format!("set_{}", name);
let concat = Ident::new(&concat, name.span());
quote! {
/// Sets the field value and returns the previous value.
pub fn #concat(&mut self, mut #name: #segment) -> #segment {
hdwalters marked this conversation as resolved.
Show resolved Hide resolved
use std::mem::swap;
swap(&mut self.#name, &mut #name);
#name
}
}
}

pub fn make_block(&self) -> TokenStream2 {
utils::make_block(&self.name, &self.functions)
}
}

impl<'a> Visit<'a> for HelperVisitor {
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));
}
}
}
}
Comment on lines +39 to +47
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
34 changes: 34 additions & 0 deletions meta/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
mod helper;
mod manager;
mod utils;

use crate::helper::HelperVisitor;
use crate::manager::ManagerVisitor;
use proc_macro::TokenStream;
use syn::visit::Visit;
use syn::*;

/// To add context manager functions to a struct, annotate the struct
/// itself with `#[derive(ContextManager)`, and required fields with
/// `#[context]`. Creates three functions `with_foo()`, `with_foo_ref()`
/// and `with_foo_fn()`.
#[proc_macro_derive(ContextManager, attributes(context))]
pub fn context_manager(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as ItemStruct);
let mut visitor = ManagerVisitor::new(&input.ident);
visitor.visit_item_struct(&input);
let output = visitor.make_block();
TokenStream::from(output)
}

/// To add setter functions designed to work with `with_foo_fn()` (see
/// above), annotate the struct itself with `#[derive(ContextHelper)`,
/// and required fields with `#[context]`.
hdwalters marked this conversation as resolved.
Show resolved Hide resolved
#[proc_macro_derive(ContextHelper, attributes(context))]
pub fn context_helper(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as ItemStruct);
let mut visitor = HelperVisitor::new(&input.ident);
visitor.visit_item_struct(&input);
let output = visitor.make_block();
TokenStream::from(output)
}
103 changes: 103 additions & 0 deletions meta/src/manager.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
use crate::utils;
use proc_macro2::TokenStream as TokenStream2;
use quote::quote;
use syn::visit::Visit;
use syn::{Field, Ident, PathSegment};

pub struct ManagerVisitor {
name: Ident,
functions: Vec<TokenStream2>,
}

impl ManagerVisitor {
pub fn new(name: &Ident) -> Self {
Self {
name: name.clone(),
functions: Vec::new(),
}
}

fn make_with(name: &Ident, segment: &PathSegment) -> TokenStream2 {
hdwalters marked this conversation as resolved.
Show resolved Hide resolved
let concat = format!("with_{}", name);
let concat = Ident::new(&concat, name.span());
quote! {
/// Sets the field value (which must implement the Copy and
/// Clone traits) and restores the previous value after the
/// body function has returned.
pub fn #concat<T, E, B>(&mut self, #name: #segment, mut body: B) -> Result<T, E>
where
B: FnMut(&mut Self) -> Result<T, E>,
{
// Native types are implicitly copied on clone.
let prev = self.#name.clone();
self.#name = #name;
let result = body(self);
self.#name = prev;
result
}
}
}

fn make_with_ref(name: &Ident, segment: &PathSegment) -> TokenStream2 {
let concat = format!("with_{}_ref", name);
let concat = Ident::new(&concat, name.span());
quote! {
/// Sets the field value by swapping the references, and
/// restores the previous value after the body function has
/// returned.
pub fn #concat<T, E, B>(&mut self, #name: &mut #segment, mut body: B) -> Result<T, E>
where
B: FnMut(&mut Self) -> Result<T, E>,
{
use std::mem::swap;
swap(&mut self.#name, #name);
let result = body(self);
swap(&mut self.#name, #name);
result
}
}
}

fn make_with_fn(name: &Ident, segment: &PathSegment) -> TokenStream2 {
let concat = format!("with_{}_fn", name);
let concat = Ident::new(&concat, name.span());
quote! {
/// Sets the field value on the encapsulated struct using
/// its member function, and restores the previous value
/// after the body function has returned.
///
/// Additionally, to add setter functions designed to work
/// with `with_foo_fn()`, annotate the encapsulated struct
/// with `#[derive(ContextHelper)`, and required fields with
/// `#[context]`.
pub fn #concat<V, S, T, E, B>(&mut self, mut setter: S, value: V, mut body: B) -> Result<T, E>
where
S: FnMut(&mut #segment, V) -> V,
B: FnMut(&mut Self) -> Result<T, E>,
{
let prev = setter(&mut self.#name, value);
let result = body(self);
setter(&mut self.#name, prev);
result
}
}
}

pub fn make_block(&self) -> TokenStream2 {
utils::make_block(&self.name, &self.functions)
}
}

impl<'a> Visit<'a> for ManagerVisitor {
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_with(name, segment));
self.functions.push(Self::make_with_ref(name, segment));
self.functions.push(Self::make_with_fn(name, segment));
}
}
}
}
hdwalters marked this conversation as resolved.
Show resolved Hide resolved
}
39 changes: 39 additions & 0 deletions meta/src/utils.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
use proc_macro2::TokenStream as TokenStream2;
use quote::quote;
use syn::{Attribute, Field, Ident, Meta, PathSegment, Type};

/// Implements multiple functions for a struct implementation block.
pub fn make_block(name: &Ident, functions: &Vec<TokenStream2>) -> TokenStream2 {
// See [https://users.rust-lang.org/t/how-to-use-a-vector-of-tokenstreams-created-with-quote-within-quote/81092].
quote! {
impl #name {
#(#functions)*
}
}
}

/// Tests whether a given field attribute is `#[context]`, for both
/// `#[derive(ContextManager)]` and `#[derive(ContextHelper)]` enhanced
/// structs.
pub fn is_context(attr: &Attribute) -> bool {
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this makes things any clearer.

Copy link
Member

@Ph0enixKM Ph0enixKM Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@hdwalters hdwalters Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// Gets the type of a given field. Note, we use the `PathSegment` not
/// the contained `Ident`, because that supports generic field types
/// like `Option<String>`.
pub fn get_type(field: &Field) -> Option<&PathSegment> {
if let Type::Path(path) = &field.ty {
path.path.segments.last()
} else {
None
}
}
40 changes: 20 additions & 20 deletions src/modules/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,28 +35,28 @@ impl SyntaxModule<ParserMetadata> for Block {
}

fn parse(&mut self, meta: &mut ParserMetadata) -> SyntaxResult {
meta.push_scope();
while let Some(token) = meta.get_current_token() {
// Handle the end of line or command
if ["\n", ";"].contains(&token.word.as_str()) {
meta.increment_index();
continue;
}
// Handle block end
else if token.word == "}" {
break;
}
let mut statement = Statement::new();
if let Err(failure) = statement.parse(meta) {
return match failure {
Failure::Quiet(pos) => error_pos!(meta, pos, "Unexpected token"),
Failure::Loud(err) => return Err(Failure::Loud(err))
meta.with_push_scope(|meta| {
while let Some(token) = meta.get_current_token() {
// Handle the end of line or command
if ["\n", ";"].contains(&token.word.as_str()) {
meta.increment_index();
continue;
}
// Handle block end
else if token.word == "}" {
break;
}
let mut statement = Statement::new();
if let Err(failure) = statement.parse(meta) {
return match failure {
Failure::Quiet(pos) => error_pos!(meta, pos, "Unexpected token"),
Failure::Loud(err) => return Err(Failure::Loud(err))
}
}
self.statements.push(statement);
}
self.statements.push(statement);
}
meta.pop_scope();
Ok(())
Ok(())
})
}
}

Expand Down
Loading