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

Reduce usage of last statement spans in proc-macros #5092

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

Veykril
Copy link
Contributor

@Veykril Veykril commented Oct 11, 2022

This excludes the initial let statement of the proc-macro expansion from receiving the last statement spans to aid completions in rust-analyzer. The current span confuses rust-analyzer as it will map the tail expression tokens to the let keyword (as this is the first token it finds with the same span) which currently breaks completions. This commit should not degrade the initial intent of the span reusages, as the type mismatch parts are still spanned appropriately.

Motivation

R-a is currently pretty terrible with spans, but needs them for proper usage of features in proc-macros, for example it uses them for figuring out where to analyse surrounding context for completions. More importantly it uses them to figure out what tokens to remove after fixing up syntax errors. Tokio's usage of the trailing expression span confuses r-a though, as all the tokens the proc-macro emits receive the span making r-a think they are partial part of the synthetic syntax it created to fix up a broken syntax tree removing them from the output.

Now this is obviously a bug in r-a, unfortunately its gonna take a while until that is gonna be fixed in r-a (as we need to rewrite our token mapping infra completely), hence this PR. If this is not something you wish to merge, that is fine as this basically only works around a very few cases of completions breaking in r-a (wherever broken syntax is typed basically), but I figured I could put the patch out given I already wrote it to test things out. See rust-lang/rust-analyzer#13355 for a lengthy discussion, and rust-lang/rust-analyzer#13388 for the fixup breakage.

Solution

The solution is simple due to technical reasons of how r-a's completion work, but if we exclude the initial let statement that re-uses the actual body, the tokens preceding the body will never be removed and thus won't mess up the ranges allowing r-a's completion to work properly with it. This does not degrade the diagnostics improvement that sparked the re-use of spans in the first place, as the type mismatches come from the final return expression, not the let statement. (Though I might be wrong, maybe there are certain cases where a diagnostic would come from the let statement?)

This excludes the initial let statement of the proc-macro expansion
from receiving the last statement spans to aid completions in rust-analyzer.
The current span confuses rust-analyzer as it will map the tail expression
tokens to the let keyword (as this is the first token it finds with the
same span) which currently breaks completions. This commit should not
degrade the initial intent of the span reusages, as the type mismatch
parts are still spanned appropriately.
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

LGTM.

@Darksonn Darksonn merged commit 37d1d09 into tokio-rs:master Oct 14, 2022
@conradludgate
Copy link
Contributor

conradludgate commented Nov 29, 2022

I have a suspicion that this cause an interesting regression, but I'm not certain.

In our codebase, we have something along the lines of

macro_rules! custom_test {
    (async fn $name:ident() $b:block) => {
        #[::tokio::test]
        async fn $name() {
            dbg!("foo");
            $b
        }
    };
}

// usage normally with macro_rules_attribute, but for demonstration
custom_test!(
    async fn foo() {
        dbg!("bar");
    }
);

This compiles with tokio-macros 1.8.0 but does not with 1.8.1

error[E0425]: cannot find value `body` in this scope
  --> tests/main.rs:74:20
   |
74 |       async fn foo() {
   |  ____________________^
75 | |         dbg!("bar");
76 | |     }
   | |_____^ not found in this scope

@taiki-e
Copy link
Member

taiki-e commented Nov 29, 2022

@conradludgate: Thanks for the report. I yanked 1.8.1 and opened #5243 to track the regression.

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