Skip `unused_parens` report for `Paren(Path(..))` in macro. by surechen · Pull Request #123314 · rust-lang/rust

surechen · 2024-04-01T06:59:51Z

fixes #120642

In following code, unused_parens suggest change <($($rest),*)>::bar() to <$rest>::bar() which will cause another err: error: variable 'rest' is still repeating at this depth：

trait Foo {
    fn bar();
}

macro_rules! problem {
    ($ty:ident) => {
        impl<$ty: Foo> Foo for ($ty,) {
            fn bar() { <$ty>::bar() }
        }
    };
    ($ty:ident $(, $rest:ident)*) => {
        impl<$ty: Foo, $($rest: Foo),*> Foo for ($ty, $($rest),*) {
            fn bar() {
                <$ty>::bar();
                <($($rest),*)>::bar()
            }
        }
        problem!($($rest),*);
    }
}

I think maybe we can handle this by avoid warning for Paren(Path(..)) in the macro. Is this reasonable approach?

rustbot · 2024-04-01T06:59:59Z

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Nadrieril · 2024-04-01T10:34:52Z

It would be better to fix the incorrect span detection. I don't know how easy that is however

cjgillot · 2024-04-07T17:20:25Z

The lint is correct to fire here, only the suggestion is wrong. Looking at this kind of code, the compiler should assume the author meant <(T3,)>::bar() (notice the comma).

surechen · 2024-04-08T09:34:01Z

The lint is correct to fire here, only the suggestion is wrong. Looking at this kind of code, the compiler should assume the author meant <(T3,)>::bar() (notice the comma).

Thank you. I agree with you. I initially wanted to do like this.
Since this lint detects the ast expr after macro expansion, In fact, I don't how to find the macro by a ast expr, and how to judge the form of the macro. I will try to do it again. If you have some idea, please give me some help.

cjgillot · 2024-04-08T23:57:57Z

In the call to emit_unused_delims, the spans argument tells where are the parentheses, and to use them for a suggestion. If spans is None, there is no suggestion.
Pedantically, we should strive to suggest <$($rest),*>::bar(), ie just remove the parentheses. Maybe using start_point/end_point instead of looking at the inner type?
From a brief look, all the calls to emit_unused_delims use a spans argument constructed the same way. So they may all suffer from the same bug. It's a good opportunity to exercise all cases in the test.

surechen · 2024-04-09T03:10:36Z

In the call to emit_unused_delims, the spans argument tells where are the parentheses, and to use them for a suggestion. If spans is None, there is no suggestion. Pedantically, we should strive to suggest <$($rest),*>::bar(), ie just remove the parentheses. Maybe using start_point/end_point instead of looking at the inner type? From a brief look, all the calls to emit_unused_delims use a spans argument constructed the same way. So they may all suffer from the same bug. It's a good opportunity to exercise all cases in the test.

Thank you very much, I will read and think about your suggestions as soon as possible

surechen · 2024-04-10T09:16:41Z

It would be better to fix the incorrect span detection. I don't know how easy that is however

Thank you very much.

I find there are already some measures to avoid reporting errors for conditions inside macros, such as https://github.com/rust-lang/rust/blob/master/compiler/rustc_lint/src/unused.rs#L1023 and https://github.com/rust-lang/rust/blob/master/compiler/rustc_lint/src/unused.rs#L1353.

Because it's hard to give correct suggestion for <($($rest),*)>::bar() to <($($rest,)*)>::bar().
Other suggestion like <$rest>::bar() or even <$($rest),*>::bar() will also cause compiler error.
So I just give error report and avoid suggestion for in macro expand.

Please see if it meets expectations?

fixes rust-lang#120642

Nadrieril · 2024-04-10T19:09:53Z

Yep, that's a fine solution. Thank you!

@bors r+

bors · 2024-04-10T19:09:56Z

📌 Commit c8490a0 has been approved by Nadrieril

It is now in the queue for this repository.

Skip `unused_parens` report for `Paren(Path(..))` in macro. fixes rust-lang#120642 In following code, `unused_parens` suggest change `<($($rest),*)>::bar()` to `<$rest>::bar()` which will cause another err: `error: variable 'rest' is still repeating at this depth`： ```rust trait Foo { fn bar(); } macro_rules! problem { ($ty:ident) => { impl<$ty: Foo> Foo for ($ty,) { fn bar() { <$ty>::bar() } } }; ($ty:ident $(, $rest:ident)*) => { impl<$ty: Foo, $($rest: Foo),*> Foo for ($ty, $($rest),*) { fn bar() { <$ty>::bar(); <($($rest),*)>::bar() } } problem!($($rest),*); } } ``` I think maybe we can handle this by avoid warning for `Paren(Path(..))` in the macro. Is this reasonable approach?

Rollup of 6 pull requests Successful merges: - rust-lang#122470 (`f16` and `f128` step 4: basic library support) - rust-lang#122954 (Be more specific when flagging imports as redundant due to the extern prelude) - rust-lang#123314 (Skip `unused_parens` report for `Paren(Path(..))` in macro.) - rust-lang#123360 (Document restricted_std) - rust-lang#123703 (Use `fn` ptr signature instead of `{closure@..}` in infer error) - rust-lang#123732 (Factor some common `io::Error` constants) r? `@ghost` `@rustbot` modify labels: rollup

Skip `unused_parens` report for `Paren(Path(..))` in macro. fixes rust-lang#120642 In following code, `unused_parens` suggest change `<($($rest),*)>::bar()` to `<$rest>::bar()` which will cause another err: `error: variable 'rest' is still repeating at this depth`： ```rust trait Foo { fn bar(); } macro_rules! problem { ($ty:ident) => { impl<$ty: Foo> Foo for ($ty,) { fn bar() { <$ty>::bar() } } }; ($ty:ident $(, $rest:ident)*) => { impl<$ty: Foo, $($rest: Foo),*> Foo for ($ty, $($rest),*) { fn bar() { <$ty>::bar(); <($($rest),*)>::bar() } } problem!($($rest),*); } } ``` I think maybe we can handle this by avoid warning for `Paren(Path(..))` in the macro. Is this reasonable approach?

Rollup of 7 pull requests Successful merges: - rust-lang#122470 (`f16` and `f128` step 4: basic library support) - rust-lang#122954 (Be more specific when flagging imports as redundant due to the extern prelude) - rust-lang#123314 (Skip `unused_parens` report for `Paren(Path(..))` in macro.) - rust-lang#123360 (Document restricted_std) - rust-lang#123661 (Stabilize `cstr_count_bytes`) - rust-lang#123703 (Use `fn` ptr signature instead of `{closure@..}` in infer error) - rust-lang#123704 (Tweak value suggestions in `borrowck` and `hir_analysis`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 8 pull requests Successful merges: - rust-lang#122470 (`f16` and `f128` step 4: basic library support) - rust-lang#122954 (Be more specific when flagging imports as redundant due to the extern prelude) - rust-lang#123314 (Skip `unused_parens` report for `Paren(Path(..))` in macro.) - rust-lang#123360 (Document restricted_std) - rust-lang#123661 (Stabilize `cstr_count_bytes`) - rust-lang#123703 (Use `fn` ptr signature instead of `{closure@..}` in infer error) - rust-lang#123756 (clean up docs for `File::sync_*`) - rust-lang#123761 (Use `suggest_impl_trait` in return type suggestion on type error) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123314 - surechen:fix_120642, r=Nadrieril Skip `unused_parens` report for `Paren(Path(..))` in macro. fixes rust-lang#120642 In following code, `unused_parens` suggest change `<($($rest),*)>::bar()` to `<$rest>::bar()` which will cause another err: `error: variable 'rest' is still repeating at this depth`： ```rust trait Foo { fn bar(); } macro_rules! problem { ($ty:ident) => { impl<$ty: Foo> Foo for ($ty,) { fn bar() { <$ty>::bar() } } }; ($ty:ident $(, $rest:ident)*) => { impl<$ty: Foo, $($rest: Foo),*> Foo for ($ty, $($rest),*) { fn bar() { <$ty>::bar(); <($($rest),*)>::bar() } } problem!($($rest),*); } } ``` I think maybe we can handle this by avoid warning for `Paren(Path(..))` in the macro. Is this reasonable approach?

rustbot assigned Nadrieril Apr 1, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 1, 2024

surechen force-pushed the fix_120642 branch from b6d90c4 to 8e9b2bb Compare April 10, 2024 08:18

This comment has been minimized.

Sign in to view

skip unused_parens's suggestion for Paren in macro.

c8490a0

fixes rust-lang#120642

surechen force-pushed the fix_120642 branch from 8e9b2bb to c8490a0 Compare April 10, 2024 09:57

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2024

fmease mentioned this pull request Apr 10, 2024

Rollup of 6 pull requests #123746

Closed

fmease mentioned this pull request Apr 10, 2024

Rollup of 7 pull requests #123755

Closed

fmease mentioned this pull request Apr 10, 2024

Rollup of 8 pull requests #123762

Merged

bors merged commit 82c6f18 into rust-lang:master Apr 11, 2024

rustbot added this to the 1.79.0 milestone Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Skip `unused_parens` report for `Paren(Path(..))` in macro.#123314

Skip `unused_parens` report for `Paren(Path(..))` in macro.#123314
bors merged 1 commit intorust-lang:masterfrom
surechen:fix_120642

surechen commented Apr 1, 2024

Uh oh!

rustbot commented Apr 1, 2024

Uh oh!

Nadrieril commented Apr 1, 2024 •

edited

Loading

Uh oh!

cjgillot commented Apr 7, 2024

Uh oh!

surechen commented Apr 8, 2024

Uh oh!

cjgillot commented Apr 8, 2024

Uh oh!

surechen commented Apr 9, 2024

Uh oh!

This comment has been minimized.

surechen commented Apr 10, 2024 •

edited

Loading

Uh oh!

Nadrieril commented Apr 10, 2024

Uh oh!

bors commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

surechen commented Apr 1, 2024

Uh oh!

rustbot commented Apr 1, 2024

Uh oh!

Nadrieril commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjgillot commented Apr 7, 2024

Uh oh!

surechen commented Apr 8, 2024

Uh oh!

cjgillot commented Apr 8, 2024

Uh oh!

surechen commented Apr 9, 2024

Uh oh!

This comment has been minimized.

surechen commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nadrieril commented Apr 10, 2024

Uh oh!

bors commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Nadrieril commented Apr 1, 2024 •

edited

Loading

surechen commented Apr 10, 2024 •

edited

Loading