From b32d57f5f2bf0c293e0dd2c7c1dd54cdf64092a6 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Tue, 5 Nov 2024 01:40:04 -0500 Subject: [PATCH] Tighten what is allowed inside the varnish mod I think it is a bad practice to allow structs and enums inside the varnish `mod` item - because it conflates implementation detail and the public interface, making it less clear what will get exported. Instead, I propose everything except `impl` blocks and functions to be dis-allowed. --- examples/vmod_be/README.md | 5 + examples/vmod_be/src/lib.rs | 14 +-- examples/vmod_event/src/lib.rs | 15 +-- examples/vmod_object/src/lib.rs | 15 +-- varnish-macros/snapshots/docs_types@code.snap | 4 +- varnish-macros/snapshots/object_obj@code.snap | 4 +- varnish-macros/src/parser.rs | 102 ++++++++++++++---- varnish-macros/tests/pass/docs.rs | 10 +- varnish-macros/tests/pass/object.rs | 8 +- 9 files changed, 124 insertions(+), 53 deletions(-) diff --git a/examples/vmod_be/README.md b/examples/vmod_be/README.md index ff42106..e1aa32f 100644 --- a/examples/vmod_be/README.md +++ b/examples/vmod_be/README.md @@ -21,6 +21,11 @@ import be from "path/to/libbe.so"; ### Object `parrot` +parrot is our VCL object, which just holds a rust Backend, +it only needs two functions: +- new(), so that the VCL can instantiate it +- backend(), so that we can produce a C pointer for varnish to use + ```vcl // Create a new instance of the object in your VCL init function sub vcl_init { diff --git a/examples/vmod_be/src/lib.rs b/examples/vmod_be/src/lib.rs index f9e428e..2c76bea 100644 --- a/examples/vmod_be/src/lib.rs +++ b/examples/vmod_be/src/lib.rs @@ -1,24 +1,24 @@ -use varnish::vcl::{Ctx, Serve, Transfer, VclError}; +use varnish::vcl::{Backend, Ctx, Serve, Transfer, VclError}; varnish::run_vtc_tests!("tests/*.vtc"); +#[allow(non_camel_case_types)] +struct parrot { + be: Backend, +} + /// a simple STRING dictionary in your VCL #[varnish::vmod(docs = "README.md")] mod be { use varnish::ffi::VCL_BACKEND; use varnish::vcl::{Backend, Ctx, VclError}; - use super::{Body, Sentence}; + use super::{parrot, Sentence}; /// parrot is our VCL object, which just holds a rust Backend, /// it only needs two functions: /// - new(), so that the VCL can instantiate it /// - backend(), so that we can produce a C pointer for varnish to use - #[allow(non_camel_case_types)] - pub struct parrot { - be: Backend, - } - impl parrot { pub fn new( ctx: &mut Ctx, diff --git a/examples/vmod_event/src/lib.rs b/examples/vmod_event/src/lib.rs index f375049..ac686fa 100644 --- a/examples/vmod_event/src/lib.rs +++ b/examples/vmod_event/src/lib.rs @@ -1,18 +1,19 @@ +use std::sync::atomic::AtomicI64; + varnish::run_vtc_tests!("tests/*.vtc"); +/// Number of times `on_event()` was called with `Event::Load`. +/// * `on_event()` will increment it and store it in the `PRIV_VCL` +/// * `loaded()` will access it and return to the VCL +static EVENT_LOADED_COUNT: AtomicI64 = AtomicI64::new(0); + /// Listen to VCL event #[varnish::vmod(docs = "README.md")] mod event { - use std::sync::atomic::AtomicI64; use std::sync::atomic::Ordering::Relaxed; use varnish::vcl::{Ctx, Event, LogTag}; - /// number of times on_event() was called with `Event::Load` - /// `on_event()` will increment it and store it in the `PRIV_VCL` - /// loaded() will access it and return to the VCL - static EVENT_LOADED_COUNT: AtomicI64 = AtomicI64::new(0); - /// Return the number of VCL loads stored during when the event function ran. pub fn loaded(#[shared_per_vcl] shared: Option<&i64>) -> i64 { shared.copied().unwrap_or(0) @@ -34,7 +35,7 @@ mod event { // we only care about load events, which is why we don't use `match` if matches!(event, Event::Load) { // increment the count in a thread-safe way - let last_count = EVENT_LOADED_COUNT.fetch_add(1, Relaxed); + let last_count = super::EVENT_LOADED_COUNT.fetch_add(1, Relaxed); if last_count == 1 { // Demo that we can fail on the second `load` event return Err("second load always fail"); diff --git a/examples/vmod_object/src/lib.rs b/examples/vmod_object/src/lib.rs index 1556f65..c276457 100644 --- a/examples/vmod_object/src/lib.rs +++ b/examples/vmod_object/src/lib.rs @@ -1,16 +1,19 @@ +use dashmap::DashMap; + varnish::run_vtc_tests!("tests/*.vtc"); +/// kv only contains one element: a String->String hashmap that can be used in parallel +#[allow(non_camel_case_types)] +pub struct kv { + storage: DashMap, +} + /// A simple string dictionary in your VCL #[varnish::vmod(docs = "README.md")] mod object { + use super::kv; use dashmap::DashMap; - /// kv only contains one element: a String->String hashmap that can be used in parallel - #[allow(non_camel_case_types)] - pub struct kv { - storage: DashMap, - } - // implementation needs the same methods as defined in the vcc, plus "new()" // corresponding to the constructor, which requires the context (_ctx) , and the // name of the object in VLC (_vcl_name) diff --git a/varnish-macros/snapshots/docs_types@code.snap b/varnish-macros/snapshots/docs_types@code.snap index 2c0c4a1..c3e42ff 100644 --- a/varnish-macros/snapshots/docs_types@code.snap +++ b/varnish-macros/snapshots/docs_types@code.snap @@ -152,6 +152,7 @@ mod types { }; const JSON: &CStr = c"VMOD_JSON_SPEC\u{2}\n[\n [\n \"$VMOD\",\n \"1.0\",\n \"types\",\n \"Vmod_vmod_types_Func\",\n \"84444b833e86e8a67f59228fa2634183f81f58c50e684ec4152fd478ab9875fe\",\n \"Varnish (version) (hash)\",\n \"0\",\n \"0\"\n ],\n [\n \"$CPROTO\",\n \"\\nstruct vmod_types_DocStruct;\\n\\ntypedef VCL_VOID td_vmod_types_with_docs(\\n VRT_CTX\\n);\\n\\ntypedef VCL_VOID td_vmod_types_no_docs(\\n VRT_CTX\\n);\\n\\ntypedef VCL_VOID td_vmod_types_doctest(\\n VRT_CTX,\\n VCL_INT,\\n VCL_INT\\n);\\n\\ntypedef VCL_VOID td_vmod_types_arg_only(\\n VRT_CTX,\\n VCL_INT\\n);\\n\\nstruct arg_vmod_types_DocStruct__init {\\n char valid_cap;\\n VCL_INT cap;\\n};\\n\\ntypedef VCL_VOID td_vmod_types_DocStruct__init(\\n VRT_CTX,\\n struct vmod_types_DocStruct **,\\n const char *,\\n struct arg_vmod_types_DocStruct__init *\\n);\\n\\ntypedef VCL_VOID td_vmod_types_DocStruct__fini(\\n struct vmod_types_DocStruct **\\n);\\n\\ntypedef VCL_VOID td_vmod_types_DocStruct_function(\\n VRT_CTX,\\n struct vmod_types_DocStruct *,\\n VCL_STRING\\n);\\n\\nstruct Vmod_vmod_types_Func {\\n td_vmod_types_with_docs *f_with_docs;\\n td_vmod_types_no_docs *f_no_docs;\\n td_vmod_types_doctest *f_doctest;\\n td_vmod_types_arg_only *f_arg_only;\\n td_vmod_types_DocStruct__init *f_DocStruct__init;\\n td_vmod_types_DocStruct__fini *f_DocStruct__fini;\\n td_vmod_types_DocStruct_function *f_DocStruct_function;\\n};\\n\\nstatic struct Vmod_vmod_types_Func Vmod_vmod_types_Func;\"\n ],\n [\n \"$FUNC\",\n \"with_docs\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_with_docs\",\n \"\"\n ]\n ],\n [\n \"$FUNC\",\n \"no_docs\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_no_docs\",\n \"\"\n ]\n ],\n [\n \"$FUNC\",\n \"doctest\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_doctest\",\n \"\",\n [\n \"INT\",\n \"_no_docs\"\n ],\n [\n \"INT\",\n \"_v\"\n ]\n ]\n ],\n [\n \"$FUNC\",\n \"arg_only\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_arg_only\",\n \"\",\n [\n \"INT\",\n \"_v\"\n ]\n ]\n ],\n [\n \"$OBJ\",\n \"DocStruct\",\n {\n \"NULL_OK\": false\n },\n \"struct vmod_types_DocStruct\",\n [\n \"$INIT\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_DocStruct__init\",\n \"struct arg_vmod_types_DocStruct__init\",\n [\n \"INT\",\n \"cap\",\n null,\n null,\n true\n ]\n ]\n ],\n [\n \"$FINI\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_DocStruct__fini\",\n \"\"\n ]\n ],\n [\n \"$METHOD\",\n \"function\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_types_Func.f_DocStruct_function\",\n \"\",\n [\n \"STRING\",\n \"key\"\n ]\n ]\n ]\n ]\n]\n\u{3}"; } + use super::DocStruct; /// doctest on a function /// with multiple lines /// # Big header @@ -163,9 +164,6 @@ mod types { /// doctest on a function pub fn doctest(_no_docs: i64, _v: i64) {} pub fn arg_only(_v: i64) {} - /// doctest for `DocStruct`. - /// This comment is ignored because we do not handle structs, only impl blocks. - pub struct DocStruct; /// doctest for `DocStruct` implementation impl DocStruct { /// doctest for `new` diff --git a/varnish-macros/snapshots/object_obj@code.snap b/varnish-macros/snapshots/object_obj@code.snap index df81d3e..d841d14 100644 --- a/varnish-macros/snapshots/object_obj@code.snap +++ b/varnish-macros/snapshots/object_obj@code.snap @@ -303,8 +303,8 @@ mod obj { }; const JSON: &CStr = c"VMOD_JSON_SPEC\u{2}\n[\n [\n \"$VMOD\",\n \"1.0\",\n \"obj\",\n \"Vmod_vmod_obj_Func\",\n \"e4dde1367a8785e0bb9e3b32a6a53880156eb4050d20f22573df4b5ea5f44461\",\n \"Varnish (version) (hash)\",\n \"0\",\n \"0\"\n ],\n [\n \"$CPROTO\",\n \"\\nstruct vmod_obj_kv1;\\n\\nstruct vmod_obj_kv2;\\n\\nstruct vmod_obj_kv3;\\n\\nstruct arg_vmod_obj_kv1__init {\\n char valid_cap;\\n VCL_INT cap;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv1__init(\\n VRT_CTX,\\n struct vmod_obj_kv1 **,\\n const char *,\\n struct arg_vmod_obj_kv1__init *\\n);\\n\\ntypedef VCL_VOID td_vmod_obj_kv1__fini(\\n struct vmod_obj_kv1 **\\n);\\n\\ntypedef VCL_VOID td_vmod_obj_kv1_set(\\n VRT_CTX,\\n struct vmod_obj_kv1 *,\\n VCL_STRING,\\n VCL_STRING\\n);\\n\\ntypedef VCL_STRING td_vmod_obj_kv1_get(\\n VRT_CTX,\\n struct vmod_obj_kv1 *,\\n VCL_STRING\\n);\\n\\nstruct arg_vmod_obj_kv2__init {\\n char valid_cap;\\n VCL_INT cap;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv2__init(\\n VRT_CTX,\\n struct vmod_obj_kv2 **,\\n const char *,\\n struct arg_vmod_obj_kv2__init *\\n);\\n\\ntypedef VCL_VOID td_vmod_obj_kv2__fini(\\n struct vmod_obj_kv2 **\\n);\\n\\nstruct arg_vmod_obj_kv2_set {\\n VCL_STRING key;\\n char valid_value;\\n VCL_STRING value;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv2_set(\\n VRT_CTX,\\n struct vmod_obj_kv2 *,\\n struct arg_vmod_obj_kv2_set *\\n);\\n\\nstruct arg_vmod_obj_kv3__init {\\n char valid_cap;\\n VCL_INT cap;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv3__init(\\n VRT_CTX,\\n struct vmod_obj_kv3 **,\\n const char *,\\n struct arg_vmod_obj_kv3__init *\\n);\\n\\ntypedef VCL_VOID td_vmod_obj_kv3__fini(\\n struct vmod_obj_kv3 **\\n);\\n\\nstruct arg_vmod_obj_kv3_set {\\n VCL_STRING key;\\n char valid_value;\\n VCL_STRING value;\\n};\\n\\ntypedef VCL_VOID td_vmod_obj_kv3_set(\\n VRT_CTX,\\n struct vmod_obj_kv3 *,\\n struct arg_vmod_obj_kv3_set *\\n);\\n\\nstruct Vmod_vmod_obj_Func {\\n td_vmod_obj_kv1__init *f_kv1__init;\\n td_vmod_obj_kv1__fini *f_kv1__fini;\\n td_vmod_obj_kv1_set *f_kv1_set;\\n td_vmod_obj_kv1_get *f_kv1_get;\\n td_vmod_obj_kv2__init *f_kv2__init;\\n td_vmod_obj_kv2__fini *f_kv2__fini;\\n td_vmod_obj_kv2_set *f_kv2_set;\\n td_vmod_obj_kv3__init *f_kv3__init;\\n td_vmod_obj_kv3__fini *f_kv3__fini;\\n td_vmod_obj_kv3_set *f_kv3_set;\\n};\\n\\nstatic struct Vmod_vmod_obj_Func Vmod_vmod_obj_Func;\"\n ],\n [\n \"$OBJ\",\n \"kv1\",\n {\n \"NULL_OK\": false\n },\n \"struct vmod_obj_kv1\",\n [\n \"$INIT\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv1__init\",\n \"struct arg_vmod_obj_kv1__init\",\n [\n \"INT\",\n \"cap\",\n null,\n null,\n true\n ]\n ]\n ],\n [\n \"$FINI\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv1__fini\",\n \"\"\n ]\n ],\n [\n \"$METHOD\",\n \"set\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv1_set\",\n \"\",\n [\n \"STRING\",\n \"key\"\n ],\n [\n \"STRING\",\n \"value\"\n ]\n ]\n ],\n [\n \"$METHOD\",\n \"get\",\n [\n [\n \"STRING\"\n ],\n \"Vmod_vmod_obj_Func.f_kv1_get\",\n \"\",\n [\n \"STRING\",\n \"key\"\n ]\n ]\n ]\n ],\n [\n \"$OBJ\",\n \"kv2\",\n {\n \"NULL_OK\": false\n },\n \"struct vmod_obj_kv2\",\n [\n \"$INIT\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv2__init\",\n \"struct arg_vmod_obj_kv2__init\",\n [\n \"INT\",\n \"cap\",\n null,\n null,\n true\n ]\n ]\n ],\n [\n \"$FINI\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv2__fini\",\n \"\"\n ]\n ],\n [\n \"$METHOD\",\n \"set\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv2_set\",\n \"struct arg_vmod_obj_kv2_set\",\n [\n \"STRING\",\n \"key\"\n ],\n [\n \"STRING\",\n \"value\",\n null,\n null,\n true\n ]\n ]\n ]\n ],\n [\n \"$OBJ\",\n \"kv3\",\n {\n \"NULL_OK\": false\n },\n \"struct vmod_obj_kv3\",\n [\n \"$INIT\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv3__init\",\n \"struct arg_vmod_obj_kv3__init\",\n [\n \"INT\",\n \"cap\",\n null,\n null,\n true\n ]\n ]\n ],\n [\n \"$FINI\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv3__fini\",\n \"\"\n ]\n ],\n [\n \"$METHOD\",\n \"set\",\n [\n [\n \"VOID\"\n ],\n \"Vmod_vmod_obj_Func.f_kv3_set\",\n \"struct arg_vmod_obj_kv3_set\",\n [\n \"STRING\",\n \"key\"\n ],\n [\n \"STRING\",\n \"value\",\n null,\n null,\n true\n ]\n ]\n ]\n ]\n]\n\u{3}"; } + use super::*; use varnish::vcl::Ctx; - pub struct kv1; impl kv1 { pub fn new(cap: Option) -> Self { kv1 @@ -314,14 +314,12 @@ mod obj { String::default() } } - pub struct kv2; impl kv2 { pub fn new(cap: Option, name: &str) -> Self { kv2 } pub fn set(&self, key: &str, value: Option<&str>) {} } - pub struct kv3; impl kv3 { pub fn new(ctx: &mut Ctx, cap: Option, name: &str) -> Self { kv3 diff --git a/varnish-macros/src/parser.rs b/varnish-macros/src/parser.rs index 1e72b62..25f456c 100644 --- a/varnish-macros/src/parser.rs +++ b/varnish-macros/src/parser.rs @@ -24,6 +24,7 @@ pub fn tokens_to_model(args: TokenStream, item_mod: &mut ItemMod) -> ProcResult< impl VmodInfo { /// Parse the `mod` item and generate the model of everything + #[expect(clippy::too_many_lines)] fn parse(params: VmodParams, item: &mut ItemMod) -> ProcResult { let mut errors = Errors::new(); let mut funcs = Vec::::new(); @@ -32,29 +33,86 @@ impl VmodInfo { let mut has_event = false; if let Some((_, content)) = &mut item.content { for item in content { - if let Item::Fn(fn_item) = item { - // a function or an event handler - let func = FuncInfo::parse( - &mut shared_types, - &mut fn_item.sig, - &fn_item.vis, - &mut fn_item.attrs, - false, - ); - if let Some(func) = errors.on_err(func) { - if matches!(func.func_type, FuncType::Event) { - if has_event { - errors.add(&fn_item.sig.ident, "Only one event handler is allowed"); - continue; + match item { + Item::Fn(fn_item) => { + // a function or an event handler + let func = FuncInfo::parse( + &mut shared_types, + &mut fn_item.sig, + &fn_item.vis, + &mut fn_item.attrs, + false, + ); + if let Some(func) = errors.on_err(func) { + if matches!(func.func_type, FuncType::Event) { + if has_event { + errors.add( + &fn_item.sig.ident, + "Only one event handler is allowed", + ); + continue; + } + has_event = true; } - has_event = true; + funcs.push(func); } - funcs.push(func); } - } else if let Item::Impl(impl_item) = item { - // an object - if let Some(obj) = errors.on_err(ObjInfo::parse(impl_item, &mut shared_types)) { - objects.push(obj); + Item::Impl(impl_item) => { + // an object + if let Some(obj) = + errors.on_err(ObjInfo::parse(impl_item, &mut shared_types)) + { + objects.push(obj); + } + } + Item::Use(_) => { /* ignore */ } + Item::Struct { .. } => { + errors.add(item, &err_msg_item_not_allowed("Structs")); + } + Item::Enum { .. } => { + errors.add(item, &err_msg_item_not_allowed("Enums")); + } + Item::Const(_) => { + errors.add( + item, + "Constants are not allowed in a `mod` tagged with `#[varnish::vmod]", + ); + } + Item::Macro(_) => { + errors.add( + item, + "Macros are not allowed in a `mod` tagged with `#[varnish::vmod]", + ); + } + Item::Mod(_) => { + errors.add(item, "Nested modules are not allowed in a `mod` tagged with `#[varnish::vmod]"); + } + Item::Static(_) => { + errors.add(item, "Static variables are not allowed in a `mod` tagged with `#[varnish::vmod]"); + } + Item::Trait(_) => { + errors.add( + item, + "Traits are not allowed in a `mod` tagged with `#[varnish::vmod]", + ); + } + Item::TraitAlias(_) => { + errors.add(item, "Trait aliases are not allowed in a `mod` tagged with `#[varnish::vmod]"); + } + Item::Type(_) => { + errors.add( + item, + "Type aliases are not allowed in a `mod` tagged with `#[varnish::vmod]", + ); + } + Item::Union(_) => { + errors.add( + item, + "Unions are not allowed in a `mod` tagged with `#[varnish::vmod]", + ); + } + _ => { + errors.add(item, "Only functions and impl blocks are allowed inside a `mod` tagged with `#[varnish::vmod]`"); } } } @@ -75,6 +133,10 @@ impl VmodInfo { } } +fn err_msg_item_not_allowed(typ: &str) -> String { + format!("{typ} are not allowed inside a `mod` tagged with `#[varnish::vmod]`. Move it to an outer scope and keep just the `impl` block. More than one `impl` blocks are allowed.") +} + impl ObjInfo { /// Parse an `impl` block and treat all public functions as object methods fn parse(item_impl: &mut ItemImpl, shared_types: &mut SharedTypes) -> ProcResult { diff --git a/varnish-macros/tests/pass/docs.rs b/varnish-macros/tests/pass/docs.rs index bfb790c..913ddb4 100644 --- a/varnish-macros/tests/pass/docs.rs +++ b/varnish-macros/tests/pass/docs.rs @@ -4,6 +4,10 @@ use varnish::vmod; fn main() {} +/// doctest for `DocStruct`. +/// This comment is ignored because we do not handle structs, only impl blocks. +pub struct DocStruct; + /// main docs /// # Big header /// ## sub header @@ -16,6 +20,8 @@ mod types { * The end */ + use super::DocStruct; + /// doctest on a function /// with multiple lines /// # Big header @@ -47,10 +53,6 @@ mod types { ) { } - /// doctest for `DocStruct`. - /// This comment is ignored because we do not handle structs, only impl blocks. - pub struct DocStruct; - /// doctest for `DocStruct` implementation impl DocStruct { /// doctest for `new` diff --git a/varnish-macros/tests/pass/object.rs b/varnish-macros/tests/pass/object.rs index 172f2fa..6c5078e 100644 --- a/varnish-macros/tests/pass/object.rs +++ b/varnish-macros/tests/pass/object.rs @@ -5,11 +5,15 @@ use varnish::vmod; fn main() {} +pub struct kv1; +pub struct kv2; +pub struct kv3; + #[vmod] mod obj { + use super::*; use varnish::vcl::Ctx; - pub struct kv1; impl kv1 { pub fn new(cap: Option) -> Self { kv1 @@ -20,7 +24,6 @@ mod obj { } } - pub struct kv2; impl kv2 { pub fn new(cap: Option, #[vcl_name] name: &str) -> Self { kv2 @@ -28,7 +31,6 @@ mod obj { pub fn set(&self, key: &str, value: Option<&str>) {} } - pub struct kv3; impl kv3 { pub fn new(ctx: &mut Ctx, cap: Option, #[vcl_name] name: &str) -> Self { kv3