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

macros: Improve IDE support #6968

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

nurmohammed840
Copy link
Contributor

@nurmohammed840 nurmohammed840 commented Nov 12, 2024

This refectory improve performance and enhance better compatibility with IDEs.

Here is two examples,

Screenshot 2024-11-13 013504
Before: Note that there is no inlay hints for function arguments.

Screenshot 2024-11-13 013208
After: Fixed inlay hint hints problem. channel(buffer: .., tokio::spawn(future: ..


Screenshot 2024-11-13 013429
Before: On syntax error. Run Test | Debug button dissipated. which is frustrating because the editor toggles back and forth while typing.

Screenshot 2024-11-13 013319
After: It fix that.


It introduce two changes:

  1. Remove spaned, Because as it's not meant to be used here.
  2. The function body is preserved as is.

@nurmohammed840
Copy link
Contributor Author

I will send a fix tomorrow.

@Darksonn Darksonn added the A-tokio-macros Area: The tokio-macros crate label Nov 12, 2024
@nurmohammed840
Copy link
Contributor Author

What’s really blocking the CI is that,

Current changes preserved async body as-is:

#[tokio::main]
async fn test_with_semicolon_without_return_type() {
    #![deny(clippy::semicolon_if_nothing_returned)]
    #[deny(clippy::semicolon_if_nothing_returned)]
    0;
}

So would convert into:

fn test_with_semicolon_without_return_type() {
    let body = async {
        #![deny(clippy::semicolon_if_nothing_returned)] // ❌
        #[deny(clippy::semicolon_if_nothing_returned)]
        0;
    };
    ...
}

I didn’t realize this, The previous implementation moved global inner attributes outside:

#[deny(clippy::semicolon_if_nothing_returned)]
fn test_with_semicolon_without_return_type() {
    let body = async {
        #[deny(clippy::semicolon_if_nothing_returned)]
        0;
    };
    ...
}

There are many ways to address this issue.
@Darksonn, The real question is, is it worth this fix? The documentation doesn’t mention this behavior.

@Darksonn
Copy link
Contributor

If you believe the output changes are acceptable, you can update the outputs to make the tests pass. See tests-build/README.md for instructions. Then we will review whether the changes are important.

But keep in mind that just because something is not explained in the documentation, that doesn't mean we can just change it. We don't want to break existing users of the macro.

@davidbarsky
Copy link
Member

Speaking as a person who works on rust-analyzer:

  • I think the span changes are probably good?
  • From a hygiene perspective, I worry about the type inference breakage. I think it's important to those to avoid breaking users.
  • From a general "how does this this macro work does it work for IDEs" perspective, having the macro return the original tokens such that the expanded (but incorrect!) macro looks mostly correct would go a long way in terms of providing a good IDE experience.

@nurmohammed840
Copy link
Contributor Author

Oh! It turns out span is used for type suggestion in return or end statements, so I'll revert the previous change.

Seems like it also fix some code suggestion: #3766

@nurmohammed840
Copy link
Contributor Author

nurmohammed840 commented Nov 14, 2024

Fixed: #6930, #6306
Fixed: #5518, when fn main() accept no args

I didn’t encounter any problems #3579, Maybe this was already fixed previously

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Changes to parse_knobs and ItemFn looks generally good to me.

tokio-macros/src/entry.rs Outdated Show resolved Hide resolved
tokio-macros/src/entry.rs Show resolved Hide resolved
@@ -112,10 +112,28 @@ error: second test attribute is supplied, consider removing or changing the orde
64 | #[std::prelude::rust_2021::test]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: second test attribute is supplied, consider removing or changing the order of your test attributes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the double tokio::test attribute are now allowed?

#[tokio::test]
#[tokio::test]

Copy link
Contributor Author

@nurmohammed840 nurmohammed840 Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[tokio::test] attribute are now allowed ?

No user will receive a hard error as before. but the error message is slightly different.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, which is the new warning for this test function? The old version of the stderr file has an error referring to L67, but the new version has no error referring to L67-L69, which this test function contains.

tokio/tests/macros_test.rs Outdated Show resolved Hide resolved
@@ -389,19 +390,118 @@ fn build_config(
config.build()
}

fn fn_without_args(mut input: ItemFn, is_test: bool, config: FinalConfig) -> TokenStream {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is wrong when code contains Self keyword. (see also #6306 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • fn method(self) { ... }
  • fn method(self: ..) { ... }
    The self keyword is considered a function argument. And will fallback to async closure. Here is a test case for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about Self that starts with an uppercase letter, not all lowercase self.

For example:

impl MyStruct {
    #[tokio::main]
    async fn func() {
        let mut this = Self::defalt(); 
        //             ^^^^

        // ...
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha! So in this case, we have to fallback to async block.

fn parse_knobs(mut input: ItemFn, is_test: bool, config: FinalConfig) -> TokenStream {
input.sig.asyncness = None;
if is_test || input.sig.inputs.is_empty() && !has_self_keyword(input.body.clone().into_iter()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the cases where Self is included in the return type, generics, or where clause will still be missed.

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 should be fine. We don't need to fallback to async block, If generics or return position contain Self keyword.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. For example, the following conversion is incorrect:

impl MyStruct {
    // From:
    // #[tokio::main]
    // async fn func() -> Self { todo!() }
    //
    // To:
    fn func() -> Self {
        async fn func() -> Self { todo!() }
        let body = func();
        // ...
    }
}
error[E0401]: can't use `Self` from outer item
 --> src/lib.rs:9:28
  |
2 | impl MyStruct {
  | ---- `Self` type implicitly declared here, by this `impl`
...
9 |         async fn func() -> Self { todo!() }
  |                            ^^^^
  |                            |
  |                            use of `Self` from outer item
  |                            refer to the type directly here instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-macros Area: The tokio-macros crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants