Compile time scss macro

[Wicpar]

Created rsass_macro sub-crate for compile time usage of the library

[Wicpar]

it would be good to properly handle the error, panicking the compilation is not necessarily good

[Wicpar]

maybe also add optional format

[kaj]

Yes, this seems useful! Thanks!

Two questions, since I'm new to creating procedural macros:

How would a crate that uses it look, should it use the rsass-macro crate directly? Or should there be a macros feature in the rsass crate that exposes it? That seems to be the more common way? But I guess the rsass-macro crate would still need to be published separatley?

I agree that any errors should be properly reported. How is that normally done, can the macro function return a Result? Or should it return successfully but with output that triggers a compiler error?

[kaj]

According to the reference:

As functions, they must either return syntax, panic, or loop endlessly. [...] Panics are caught by the compiler and are turned into a compiler error. [...]

Procedural macros [...] have the same resources that the compiler has. For example, standard input, error, and output are the same that the compiler has access to. [...]

Procedural macros have two ways of reporting errors. The first is to panic. The second is to emit a compile_error macro invocation.

So a panic is ok, maybe it could be made more helpfull by writing rsass error message before the panic, or maybe the rsass error message could be the panic string. Or the same could apply with a compile_error!(..) output.

[kaj]

Introducing an error in the test, I see that the error message looks surprisingly good as-is:

error: proc macro panicked
  --> tests/test_macro.rs:4:31
   |
4  |       const CSS: &'static str = rsass_macro::scss!(r#"
   |  _______________________________^
5  | |         .class {
6  | |             background: red;
7  | |             &:hover {
...  |
10 | |         }
11 | |     "#);
   | |_______^
   |
   = help: message: called `Result::unwrap()` on an `Err` value: blue is not a number.
             ,
           5 |                 background: min(blue, 1px);
             |                             ^^^^^^^^^^^^^^
             '
             - 5:29  root stylesheet

error: could not compile `rsass-macro` due to previous error

But using the syn::Error, i improved it to:

error: blue is not a number.
         ,
       5 |                 background: min(blue, 1px);
         |                             ^^^^^^^^^^^^^^
         '
         - 5:29  root stylesheet
  --> tests/test_macro.rs:4:31
   |
4  |       const CSS: &'static str = rsass_macro::scss!(r#"
   |  _______________________________^
5  | |         .class {
6  | |             background: red;
7  | |             &:hover {
...  |
10 | |         }
11 | |     "#);
   | |_______^
   |
   = note: this error originates in the macro `rsass_macro::scss` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `rsass-macro` due to previous error

... At least I think that's an improvement. :-)

It could be improved further by creating a rust Span from combining the rsass span in the error with the rust Span::call_site(), but that would probably be rather complicated.

My modified code for giving the above error format looks like this:

#[proc_macro]
pub fn scss(tokens: TokenStream) -> TokenStream {
    let input = parse_macro_input!(tokens as LitStr);
    let format = Format {
        style: Style::Compressed,
        precision: 10,
    };
    match rsass::compile_scss(input.value().as_bytes(), format) {
        Ok(output) => {
            let output = core::str::from_utf8(&output).unwrap();
            LitStr::new(output, Span::call_site())
                .to_token_stream()
                .into()
        }
        Err(err) => {
            let msg = format!("{err:?}");
            Error::new(Span::call_site(), msg)
                .into_compile_error()
                .into()
        }
    }
}

Edit: For the Error, input.span() may be used instead of Span::call_site(). That changes the error message, but I'm not quite sure if I consider it an improvement or not ..?

[Wicpar]

Or should there be a macros feature in the rsass crate that exposes it?

that's unfortunately not possible due to circular dependencies, An additional crate is required, be it a core crate, or a macro crate.

changing the PR to your code,

[Wicpar]

i have not done all that is necessary to publish the macro subcrate, i'm not quite familiar with that process.

[kaj]

Or should there be a macros feature in the rsass crate that exposes it?

that's unfortunately not possible due to circular dependencies, An additional crate is required, be it a core crate, or a macro crate.

Ah, of course. And there has to be a separate crate since a single crate can't be both a regular lib crate and a proc-macro lib crate at the same time? (And rsass is currently both a regular lib and a bin crate.)

changing the PR to your code,

Ok.

i have not done all that is necessary to publish the macro subcrate, i'm not quite familiar with that process.

The first step would be include it in CI, but maybe I can experiment with that after merging. Also, the code is a bit crowded as it is (with the library, the cli, and a test updater in the same src dir), so maybe I should pull out those to separate crates as well, converting this git repo to a monorepo for those. But that is also easiest to think about after merging this, I believe.

But before that, are we happy with the macro api? Just taking a string as a single argument seems fine for the base case, but maybe we also want the format to be changeable, and maybe the a base path for any include files (@use, etc)? Maybe also a possibility to set variables from the macro call to use in the sass code? But then, I guess all those could be done "var-args" style, maybe with a $ sign for defining variables. If so, it should be possible to add all that later, without breaking compatibility for the case where the input is only a literal string.

[kaj]

Maybe amend the root .gitignore in the project instead of adding a separate one here?

[kaj]

These extra feutures don't seem to be needed? At least, I can run cargo test without them?

[Wicpar]

yes, i over required those. if it compiles they can be removed.

[Wicpar]

If so, it should be possible to add all that later, without breaking compatibility for the case where the input is only a literal string.

that was my logic. it can quite easily be done with syn, simply create a struct and implement the parse trait. parse the LitStr like it used to and then you can parse whatever you want, even param = value syntax.
My contributon is simply changes i made for myself, i don't have the time to get too in depth on the implementation.
You can change the PR as you like, but unfortunately i will not be able to deliver you the perfect PR that can simply be merged and published... This PR can be merged on another branch if need be, so all necessary changes can happen.

[kaj]

I'll just merge this now, and then start a separate PR for integrating it in the workspace etc.