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"] }
47 changes: 47 additions & 0 deletions meta/src/helper.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
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! {
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.

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

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

#[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)
}

#[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)
}
89 changes: 89 additions & 0 deletions meta/src/manager.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
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! {
pub fn #concat<B>(&mut self, #name: #segment, mut body: B) -> SyntaxResult
where
B: FnMut(&mut Self) -> SyntaxResult,
hdwalters marked this conversation as resolved.
Show resolved Hide resolved
{
// 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! {
pub fn #concat<B>(&mut self, #name: &mut #segment, mut body: B) -> SyntaxResult
where
B: FnMut(&mut Self) -> SyntaxResult,
{
use std::mem::swap;
swap(&mut self.#name, #name);
let result = body(self);
swap(&mut self.#name, #name);
result
}
}
hdwalters marked this conversation as resolved.
Show resolved Hide resolved
}

fn make_with_fn(name: &Ident, segment: &PathSegment) -> TokenStream2 {
let concat = format!("with_{}_fn", name);
let concat = Ident::new(&concat, name.span());
quote! {
pub fn #concat<V, S, B>(&mut self, mut setter: S, value: V, mut body: B) -> SyntaxResult
where
hdwalters marked this conversation as resolved.
Show resolved Hide resolved
S: FnMut(&mut #segment, V) -> V,
B: FnMut(&mut Self) -> SyntaxResult,
{
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
}
32 changes: 32 additions & 0 deletions meta/src/utils.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
use proc_macro2::TokenStream as TokenStream2;
use quote::quote;
use syn::{Attribute, Field, Ident, Meta, PathSegment, Type};

// https://users.rust-lang.org/t/how-to-use-a-vector-of-tokenstreams-created-with-quote-within-quote/81092
hdwalters marked this conversation as resolved.
Show resolved Hide resolved
pub fn make_block(name: &Ident, functions: &Vec<TokenStream2>) -> TokenStream2 {
quote! {
impl #name {
#(#functions)*
}
}
}

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.

}

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.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