scylla-macros: increase hygiene #628

wmitros · 2023-01-13T15:54:08Z

While trying to make my macros at https://github.com/wmitros/scylla-rust-udf/pull/1 more hygienic, I noticed that I also need the macros here to be more hygienic.
I'm posting this mostly to find out if I'm heading in the right direction with the changes, although this patch does pass tests if it turns out being close to a state ready for merging

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
I added relevant tests for new features and bug fixes.
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
~~- [ ] I added appropriate Fixes: annotations to PR description.~~

cvybhu · 2023-01-13T17:35:45Z

Please rebase on top of latest main to fix the CI checks

piodul

I think the amount of changes that need to be done here can be reduced. Instead of fully qualifying everything, which is tedious, you could import some items with the use directive. As long as you are importing in a scope created by the macro which does not pollute the global namespace, it should be fine. For example, importing inside a method body created by the derive macro is OK, but importing items just before the impl block is not.

It would also be nice to have a test that verifies that the macros are indeed hygienic. It shouldn't be hard to add, just create an integration test (in scylla-cql or scylla crate) that uses #[no_implicit_prelude] and use the derive macros there. The scylla crate would be an exception, you will have to import it. Ref: https://gist.github.com/Kestrer/8c05ebd4e0e9347eb05f265dfb7252e1#test-your-macro-with-no_implicit_prelude

scylla-macros/src/from_row.rs

scylla-macros/src/into_user_type.rs

To make the macros more hygienic, add full paths to types in the generated code, so that the macros will still work even if the user uses the same names with different meanings in their code.

wmitros · 2023-01-17T12:55:35Z

In the last rebase, I replaced the full paths with use statements in every place it shortened the code, adjusted the get_path function so that it does not allow renaming the crate multiple times, and added a no_implicit_prelude test for renaming and just using the derive macros

piodul · 2023-01-18T09:28:26Z

scylla-macros/src/parser.rs

+                if !other.path().is_ident("scylla_crate") {
+                    continue;
+                }


It looks like this check could be extracted to the beginning of the for loop and the one done in Meta::NameValue branch removed?

piodul · 2023-01-18T09:42:58Z

scylla-macros/src/from_row.rs

-            -> ::std::result::Result<Self, scylla::cql_to_rust::FromRowError> {
-                use scylla::frame::response::result::CqlValue;
-                use scylla::cql_to_rust::{FromCqlVal, FromRow, FromRowError};
+        impl #impl_generics #path::_macro_internal::FromRow for #struct_name #ty_generics #where_clause {


Having to mention _macro_internal every time sounds redundant. Could the get_path function return a path with _macro_internal appended? It will save some typing and prevent mistakes if somebody forgets to append _macro_internal.

I was expecting that maybe we will use also some paths not including _macro_internal but we ended up not using any, so I added it in get_path and removed elsewhere

piodul · 2023-01-18T09:47:45Z

scylla/tests/hygiene.rs

@@ -0,0 +1,64 @@
+#![no_implicit_prelude]
+
+extern crate scylla as _scylla;


It would be good to have a similar test for scylla_cql, to make sure that the _macro_internal module exports everything properly. Preferably, without code duplication - maybe via a declarative macro?

I added a test for the scylla_cql crate, but I'm not completely sure it was what you expected, please take a look

piodul · 2023-01-18T10:00:12Z

scylla-macros/src/parser.rs

+    match this_path {
+        Some(path) => Ok(path),
+        None => Ok(quote::quote!(scylla)),
+    }


Nit: this_path.unwrap_or_else(|| quote::quote!(scylla));

piodul · 2023-01-18T10:01:37Z

scylla/tests/hygiene.rs

+
+    let sv = test_struct.serialized().unwrap().into_owned();
+    let sv2 = tuple_with_same_layout.serialized().unwrap().into_owned();
+    assert_eq!(sv, sv2,);


Nit: comma after sv2 is unnecessary

piodul · 2023-01-18T10:01:57Z

scylla/tests/hygiene.rs

+    let test_struct = TestStruct { a: 16 };
+    fn get_row() -> Row {
+        Row {
+            columns: ::std::vec![Some(CqlValue::Int(16)),],


Nit: comma before the ] is unnecessary

Ok, I wonder why cargo fmt didn't catch that

Currently, the macros will only work, if the code using them has a dependency on the scylla driver, or on the scylla_cql if it's renamed to scylla. This is caused by the fact that the paths referring to the scylla code are hardcoded to use a `scylla` crate. As a result, if a someone uses the `scylla_cql` crate without renaming, or renames `scylla_cql` or `scylla` to a name differen name than `scylla`, the macros will not work. Similarily, when another crate re-exports the macros, or uses them in their own macros, it can only work if the crate is used under the name `scylla` and it additionally re-exports parts of the `scylla_cql` code that are used in the macro under the same paths. This patch adds a `scylla_crate` helper attribute that allows users to set the crate name that will export the code that is used in the macros. All the symbols that are used there are also additionally exported in a `_macro_internal` module to make re-exporting important code segments more convenient.

Add a test that confirms that the macros work even if the crate was renamed. The test checks both scylla and scylla_cql crates. The test found missing `use ::std::iter...` statements, so they're also added.

Thanks to scylladb#628, it is now possible to provide an explicit path to a crate which provides necessary items for the FromRow macro. It is now possible to use this macro from the `scylla` crate itself, as we just need to point it to the `scylla-cql` crate instead. The NodeInfoRow struct is introduced to represent rows fetched from system.local and system.peers. It looks nicer than using a 5-element tuple and will reduce some noise when performing refactors in the commits that follow.

wmitros force-pushed the macros-hygiene branch from 1b49fed to 394e91b Compare January 16, 2023 09:51

piodul requested changes Jan 16, 2023

View reviewed changes

scylla-macros/src/from_row.rs Outdated Show resolved Hide resolved

scylla-macros/src/into_user_type.rs Show resolved Hide resolved

scylla-macros: add fully qualified names to all types in generated code

bc3478b

To make the macros more hygienic, add full paths to types in the generated code, so that the macros will still work even if the user uses the same names with different meanings in their code.

wmitros force-pushed the macros-hygiene branch from 394e91b to 28f6a20 Compare January 17, 2023 12:49

wmitros marked this pull request as ready for review January 17, 2023 12:56

piodul reviewed Jan 18, 2023

View reviewed changes

wmitros added 2 commits January 24, 2023 13:52

tests: assert that using macros in the renamed crate works

8bf5493

Add a test that confirms that the macros work even if the crate was renamed. The test checks both scylla and scylla_cql crates. The test found missing `use ::std::iter...` statements, so they're also added.

wmitros force-pushed the macros-hygiene branch from 28f6a20 to 8bf5493 Compare January 24, 2023 12:52

piodul approved these changes Jan 26, 2023

View reviewed changes

piodul merged commit 9a80008 into scylladb:main Jan 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scylla-macros: increase hygiene #628

scylla-macros: increase hygiene #628

wmitros commented Jan 13, 2023 •

edited

Loading

cvybhu commented Jan 13, 2023

piodul left a comment

wmitros commented Jan 17, 2023

piodul Jan 18, 2023

wmitros Jan 24, 2023

piodul Jan 18, 2023

wmitros Jan 24, 2023

piodul Jan 18, 2023

wmitros Jan 24, 2023

piodul Jan 18, 2023

wmitros Jan 24, 2023

piodul Jan 18, 2023

wmitros Jan 24, 2023

piodul Jan 18, 2023

wmitros Jan 24, 2023

		@@ -0,0 +1,64 @@
		#![no_implicit_prelude]

		extern crate scylla as _scylla;

scylla-macros: increase hygiene #628

scylla-macros: increase hygiene #628

Conversation

wmitros commented Jan 13, 2023 • edited Loading

cvybhu commented Jan 13, 2023

piodul left a comment

Choose a reason for hiding this comment

wmitros commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmitros commented Jan 13, 2023 •

edited

Loading