Add system for creating component attributes + new `#[component]` attribute

[tigerros]

I'm sorry in advance for potentially bad code! I'm still not that comfortable with Rust.

The details of the actual functionality can be found here: packages/core-macro/src/component_body/mod.rs.

Here's an overview:

• New #[component] attribute. What it does:
  • Renames component function name to PascalCase from snake_case, without generating any warnings. However, everything else other than the function name will still raise a warning if it's not snake_case
  • Automatically uses #[inline_props] if there's more than 1 parameter in the function.
  • Maybe more stuff in the future. What I wanted is to have a more "standardized" way of creating components.
• New system for making component macros. Both #[component] and #[inline_props] use this. It allows macros to take arguments, use each other, and eliminates repetition. It also verifies the validity of the component function. Right now that only includes checking if:
  • The component has a Scope argument.
  • The component has an Element return type.
• Add appropriate spans for errors. That means that errors will highlight the section of code that is causing the error, instead of just highlighting the macro. This doesn't apply everywhere. The only macros I touched were #[inline_props], #[derive(Props)] and #[component].
• Add global paths in quote!s (:: before each path) in packages/core-macro/src/props/mod.rs. This should fix #1445.
• Fix some typos.

[tigerros]

The failing checks all seem to be from different packages than core-macro. So not really my fault.

[ealmloff]

The failing checks all seem to be from different packages than core-macro. So not really my fault.

Core macro is re-exported in the dioxus crate which is used in some tests. This PR doesn't cause any errors within the core macro crate but it does cause errors in users code when you use the core macro crate. It look like several things are broken in the macro. One of these issue is this PR doesn't apply type bounds to generic the inline props macro (causing this example to break)

[tigerros]

Core macro is re-exported in the dioxus crate which is used in some tests. This PR doesn't cause any errors within the core macro crate but it does cause errors in users code when you use the core macro crate. It look like several things are broken in the macro. One of these issue is this PR doesn't apply type bounds to generic the inline props macro (causing this example to break)

My bad. I guess at some point I removed the where clause parsing and forgot to add it back. I assume that will clear up a lot of the errors. However, before I start fixing the errors, is this a feature that you would merge?

[tigerros]

Yeah, that's all the errors gone. The only check not passing is Clippy, because LinkProps is apparently too complex, but I certainly didn't change it. However, I did change the Props macro, but I only added fully qualified paths, so that bugs like the mentioned #1445 wouldn't happen. Could that be why?

[ealmloff]

Yeah, that's all the errors gone. The only check not passing is Clippy, because LinkProps is apparently too complex, but I certainly didn't change it. However, I did change the Props macro, but I only added fully qualified paths, so that bugs like the mentioned #1445 wouldn't happen. Could that be why?

Maybe the extra few characters trigger the lint? We can just allow(complex_types) for the link props

[tigerros]

I don't know if renaming the function to PascalCase is the right choice here.

You're probably right. I was just thinking that PascalCase function = bad, but I suppose with the warning system it doesn't matter.

I love the lints! but this can stop some valid code from compiling if users use type aliases. We should add a way to opt out of these lints.

Also true. I didn't really consider this because I don't think anyone would create a type alias for Scope or Element, but it wouldn't hurt to add an argument that disables it, just in case.

Tasks

• [x] Annotate LinkProps with #[allow(clippy::type-complexity)].
• [x] Remove snake_case -> PascalCase converter, but rather enforce PascalCase.
• [ ] Add argument that disables type lints. There's already an argument that disables the snake_case enforcer lint and conversion to PascalCase, so I think it should be kept separate.
• [x] Put the second function inside the main one instead of besides it. Right now this:

  #[component]
  fn MyFunction(cx: Scope) -> Element {
  todo!()
  }

  Expands to this:

  
  #[allow(non_snake_case)]
  fn MyFunction(cx: Scope) -> Element {
  todo!()
  }

[inline(always)]

fn __my_function(cx: Scope) -> Element { todo!() }

but putting the second one inside the first one would be better.

[tigerros]

Perhaps a lint + argument system would also be good.

[tigerros]

Adding #[allow(clippy::type-complexity)] doesn't satisfy Clippy. I assume it's because LinkProps isn't the problem, but rather a type generated by derive(Props) is. I'll allow the lint for the entire file. Not a fan, but I don't know how else to do it.

[ealmloff]

Snake case components that are allowed in rsx because they have at least one _ now require no_case_check:

#[component(no_case_check)]
fn valid_component(cx: Scope) -> Element {
    render! {
        div{}
    }
}

[tigerros]

Snake case components that are allowed in rsx because they have at least one _ now require no_case_check:

Actually, that might not be final. I think it might be a better idea to unify this under a lint system which the ComponentBody uses. That way every macro that uses ComponentBody has the same lints. No documentation repetition, it's going to be super easy to create new lints and they're not just going to be randomly scattered in the code (like they are now).

[tigerros]

Here's what I mean:

use syn::{ItemFn, Error};

pub trait ComponentLint {
    // None => Lint passed, Some(error) => Error
    fn passes(&self, source: &ItemFn) -> Option<Error>;
}

pub struct ComponentLinter {
    pub lints: Vec<Box<dyn ComponentLint>>,
}

impl ComponentLinter {
    pub fn passes(&self, source: &ItemFn) -> Option<Error> {
        for lint in &self.lints {
            let error = lint.passes(source);

            if error.is_some() {
                return error;
            }
        }

        None
    }
}

struct CaseCheckLint;

impl ComponentLint for CaseCheckLint {
    fn passes(&self, source: &ItemFn) -> Option<Error> {
        let ident = &source.sig.ident;

        if !is_pascal_case(&ident.to_string()) {
            Some(Error::new(ident.span(), "lint::ComponentLint - This component does not use PascalCase."))
        } else {
            None
        }
    }
}

fn is_pascal_case(input: &str) -> bool {
    // ...
}

Then, ComponentBody:

impl ComponentBody {
    // Instead of the `Parse` trait
    pub fn from(item_fn: ItemFn, linter: ComponentLinter) -> Result<ComponentBody> {
        if let Some(error) = linter.passes(item_fn) {
            return Err(error);
        }
        // ...
    }
}

The passes function has a bad name, but it's just a prototype. I personally think this is better, but I'm not the reviewer :) I imagine specifying lints would look something like this:

type ElemAlias<'a> = Element<'a>;

#[component(lint::TypeCheckLint, lint::CaseCheckLint)]
fn valid_component(cx: Scope) -> ElemAlias {
    render! { "No lints" }
}

[ealmloff]

That looks nice! Reusing the allow(snake_case_lint_name), warn(snake_case_lint_name) (using this pattern), and deny(snake_case_lint_name) syntax from normal rust could it a bit easier to learn. There is a lot of overlap here with the dioxus-check crate. With that architecture it seems fairly easy to reuse the visitor in the check crate inside of the component macro for linting.

[tigerros]

1 change: It would let us integrate dx check (for the rules of hooks) into the macro itself

2 change: Reusing the allow(snake_case_lint_name), warn(snake_case_lint_name), and deny(snake_case_lint_name) syntax

So, from this I'm getting the notion that something like this is the desired result:

type ElemAlias<'a> = Element<'a>;

// 2 change:
#[component(allow(no_element_return))]
pub fn App(cx: Scope) -> ElemAlias {
    render! { "..." }
}

// 2 change:
#[component(allow(no_pascal_case_name), warn(no_element_return))]
pub fn snake_case(cx: Scope) -> Element {
    let you_are_happy = true;
    let you_know_it = true;

    if you_are_happy && you_know_it {
      let something = use_state(cx, || "hands");
      //              ^^^^^^^^^
      // hook called conditionally: `use_state` (inside `if`)
      // note: `if you_are_happy && you_know_it { … }` is the conditional

      // 1 change: The above error is caused by the macro, not a command like `dx check`
      println!("clap your {something}");
    }

    render! { "..." }
}

Or a separate #[component_lint] macro so that it's easier to understand what's being allowed/other controls.

[tigerros]

The problem is that proc macros are quite limited, since they run before rustc. That's why, for example, aliases cause issues. Maybe it would be better to ditch the macro lints and instead make a Dioxus fork of Clippy. That would also allow us to make warning lints, since I'm not aware of a way to raise warnings in proc macros.

[ealmloff]

So, from this I'm getting the notion that something like this is the desired result: ... The problem is that proc macros are quite limited, since they run before rustc. That's why, for example, aliases cause issues. Maybe it would be better to ditch the macro lints and instead make a Dioxus fork of Clippy.

Yes, dx check could use some of the internals of clippy's analysis. Right now, the analysis is very local which makes it possible to integrate with the component macro for users that do not have the CLI installed/want quicker feedback from rust analyzer. If we switch to a more global form of analysis like clippy, integrating with the component macro makes less sense. Alases are not a huge issue, it would just be nice if there was some way to opt out of part of the linting when aliases cause issues

[tigerros]

If we switch to a more global form of analysis like clippy, integrating with the component macro makes less sense.

Of course, but it would be more reliable and powerful. There's things which the component macro can do just fine, but it is limited. With Clippy, we could use it for basically everything. As I mentioned, warnings for one are something that macros can't do, but warnings are something Dioxus would benefit from, with it's custom rsx syntax.

[ealmloff]

The problem is that proc macros are quite limited, since they run before rustc. That's why, for example, aliases cause issues. Maybe it would be better to ditch the macro lints and instead make a Dioxus fork of Clippy. That would also allow us to make warning lints, since I'm not aware of a way to raise warnings in proc macros.

You can create an very ugly, much less readable version of warnings in a macro:

#[deprecated(note = "this is a custom warning")]
#[allow(unused)]
struct CustomWarning;

const _: CustomWarning = CustomWarning;

[tigerros]

You can create an very ugly, much less readable version of warnings in a macro:

That's why I'm suggesting Clippy :D There's also the fact that with Clippy we wouldn't need to worry about aliases or weird imports.

[tigerros]

That's why I'm suggesting Clippy

@ealmloff Any opinion on this? If you don't want to use Clippy, I can make the lint system in the macro, but right now I'm just waiting on your input.

[ealmloff]

You can create an very ugly, much less readable version of warnings in a macro: That's why I'm suggesting Clippy :D There's also the fact that with Clippy we wouldn't need to worry about aliases or weird imports.

That's why I'm suggesting Clippy

@ealmloff Any opinion on this? If you don't want to use Clippy, I can make the lint system in the macro, but right now I'm just waiting on your input.

Clippy would definitely be better, but a bit more difficult. If we can get Clippy working, linting in the macro isn't needed. Not including a lint system in this PR, and working on a Clippy-backed system in another PR might be the best option.

[tigerros]

Not including a lint system in this PR, and working on a Clippy-backed system in another PR might be the best option.

Okay then. I'll remove type checking (cx: Scope, -> Element), but I'll keep the functionality that allows a PascalCase component with no warnings, since that was why made this PR in the first place. Edit: I'll keep some of the type checking. If the component has a &self argument/no argument at all or if the function doesn't return anything at all, it's safe to throw an error. No aliases or path hijinks are in play there.

[tigerros]

Clippy is failing because I made the #[inline_props] macro deprecated, pointing out that users should instead use #[component]. My reasoning is that the #[component] macro:

• Automatically derives props if there's more than 1 argument.
• Has some additional features.
• Has a cleaner name.

For the reasons above, I would also like to remove #[inline_props] completely in the future, but that's too drastic for now. Otherwise, it's ready to merge! I'll update the docs and occurences of #[inline_props] when it's merged.

[ealmloff]

For the reasons above, I would also like to remove #[inline_props] completely in the future, but that's too drastic for now. Otherwise, it's ready to merge!

Depreciating #[inline_props] makes sense. We should keep it in for at least one major release before completely removing it because of how common it is

I'll update the docs and occurrences of #[inline_props] when it's merged.

I try to avoid merging PRs that have checks failing. I understand why clippy is failing and I agree it isn't much of an issue, but if I merge this without checks passing, all new PRs before you create your fix PR will have tests failing for a unrelated reason which makes finding PRs that are ready for a review more difficult

[tigerros]

I try to avoid merging PRs that have checks failing

I understand. I'll replace it then. I'll also update the docsite and create a PR there. Otherwise, does the state of this look good?

[tigerros]

I changed all references of #[inline_props] in examples/ and packages/ to #[component]. I also sometimes removed #![allow(non_snake_case)] but not always. Anyway, there's still references to #[inline_props] in the docs/ directory, but that's going to be removed, so I'm not sure what to do there. Should I just remove the folder? I can copy the changes from #1408 then push them here to fix Clippy.

[ealmloff]

I changed all references of #[inline_props] in examples/ and packages/ to #[component]. I also sometimes removed #![allow(non_snake_case)] but not always. Anyway, there's still references to #[inline_props] in the docs/ directory, but that's going to be removed, so I'm not sure what to do there. Should I just remove the folder? I can copy the changes from #1408 then push them here to fix Clippy.

Either one works. Both removing the folder or switching inline_props to components works should fix it. If you remove the docs folder, I can just close my original PR

[tigerros]

Removed the folder. Did all the tests on my machine, so it should be good.