Avoid wrong code suggesting for attribute macro #107254

chenyukang · 2023-01-24T11:41:29Z

m-ou-se · 2023-01-24T13:03:08Z

It'd be nice if we can find a more general solution.

I don't think there should ever be a suggestion using a call_site span of an attribute macro.

tests/ui/coercion/coerce-block-tail-83783.stderr

chenyukang · 2023-01-24T20:58:55Z

it is right, there is a new .fixed file. it seems strange, but it is valid code. Michael Goulet ***@***.***> 于 2023年1月25日周三 01:58写道：

…

***@***.**** commented on this pull request. ------------------------------ In tests/ui/coercion/coerce-block-tail-83783.stderr <#107254 (comment)>: > @@ -6,6 +6,10 @@ LL | _consume_reference::<i32>(&async { Box::new(7_i32) }.await); | = note: expected type `i32` found struct `Box<i32>` +help: consider unboxing the value + | +LL | _consume_reference::<i32>(&*async { Box::new(7_i32) }.await); This is not right? — Reply to this email directly, view it on GitHub <#107254 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABYJ5U6YYHI6FO4NFW6U5TWUAJ57ANCNFSM6AAAAAAUE7HPFI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

chenyukang · 2023-01-25T10:05:38Z

compiler/rustc_hir_typeck/src/demand.rs

@@ -706,6 +706,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
        expected: Ty<'tcx>,
        expr_ty: Ty<'tcx>,
    ) -> bool {
+        if in_external_macro(self.tcx.sess, expr.span) {


@compiler-errors
My change at here makes this case failed:

rustc --edition 2021 tests/ui/async-await/proper-span-for-type-error.rs error[E0308]: mismatched types --> tests/ui/async-await/proper-span-for-type-error.rs:8:5 | 8 | a().await //~ ERROR mismatched types | ^^^^^^^^^ expected enum `Result`, found `()` | = note: expected enum `Result<(), i32>` found unit type `()` help: try adding an expression at the end of the block | 8 ~ a().await; 9 ~ Ok(()) //~ ERROR mismatched types |

This is because in_external_macro is not right, missing DesugaringKind::Await, so I added it and with extra DesugaringKind::Async.

So the new suggestion comes out:

_consume_reference::<i32>(&*async { Box::new(7_i32) }.await);

It seems right, so I keep it.

By the way, I think our current in_external_macro still missing some DesugaringKind, and the in_external_macro is not a proper name right now considering it actually not just checking Macro, a suitable function name should be something like proper_for_suggesting.

And we have a similar function in span, but it definitely need some fix:

rust/compiler/rustc_span/src/lib.rs

Lines 600 to 607 in c048326

pub fn can_be_used_for_suggestions(self) -> bool {

!self.from_expansion()

// FIXME: If this span comes from a `derive` macro but it points at code the user wrote,

// the callsite span and the span will be pointing at different places. It also means that

// we can safely provide suggestions on this span.

|| (matches!(self.ctxt().outer_expn_data().kind, ExpnKind::Macro(MacroKind::Derive, _))

&& self.parent_callsite().map(|p| (p.lo(), p.hi())) != Some((self.lo(), self.hi())))

}

We need some cleanup for these functions here?

chenyukang · 2023-01-25T10:55:10Z

It'd be nice if we can find a more general solution.

I don't think there should ever be a suggestion using a call_site span of an attribute macro.

Yes, we'd better find a centralized way to do this since it's relevant for pretty much all reported diagnostics.

This is the second time I solve this kind of issue, I tried to add a span.can_be_used_for_suggestions as filter in this function:

rust/compiler/rustc_errors/src/diagnostic.rs

Line 625 in c048326

pub fn multipart_suggestion_with_style(

and

rust/compiler/rustc_errors/src/diagnostic.rs

Line 707 in c048326

pub fn span_suggestion_with_style(

There are about 10+ testcases's result changed, I need to tweak the filter function.
I'm not sure whether this is the right direction, or there is any better way?
@compiler-errors @estebank

chenyukang · 2023-04-29T13:00:26Z

@compiler-errors do you have more comments for this PR?
I haven't figured out a better general solution yet, I think we may merge this at the moment.

estebank · 2023-05-04T04:40:46Z

@chenyukang I believe that a more general solution would be something along the lines of my attempt at #109082. The problem is that that PR causes a perf regression that I haven't yet been able to shake off. We could land this PR as is (and the change to in_external_macro is correct), but feels like adding yet another corner case check when we could nip this entire category of issues in the bud. Would you be interested in taking on the strategy shown in #109082 and see if we can do that without the perf impact?

chenyukang · 2023-05-05T18:50:33Z

@estebank I will take a look at your PR, I guess the performance regression is introduced by the changes on some functions of Span.

bors · 2023-07-14T18:10:25Z

☔ The latest upstream changes (presumably #113591) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC · 2023-08-02T05:57:58Z

@chenyukang any updates on this?

chenyukang · 2023-08-02T07:04:21Z

@chenyukang any updates on this?

I just resolved the conflicts, as @estebank said we may land this PR but #109082 is an more completed solution.

#109082 has some perf issue right now. I may also take a look at it.
@estebank would you please approve this PR?

rustbot · 2023-08-02T08:33:04Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

chenyukang · 2023-08-02T08:38:08Z

Fix the issue of in_external_macro will make lint checker RedundantLocals fail for async and await.

So I added a another commit to fix it:
ac25636

ref here:

rust/compiler/rustc_ast_lowering/src/item.rs

Line 1052 in 7a5d2d0

// async fn foo(<pattern>: <ty>, <pattern>: <ty>, <pattern>: <ty>) {

estebank · 2023-08-02T12:40:51Z

@bors r+

bors · 2023-08-02T12:40:53Z

📌 Commit ac25636 has been approved by estebank

It is now in the queue for this repository.

bors · 2023-08-02T15:27:58Z

⌛ Testing commit ac25636 with merge 07ac64019fac2eba26112cea9e1a27069103b010...

bors · 2023-08-02T17:22:10Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-08-02T17:22:12Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

estebank · 2023-08-02T18:56:33Z

@bors retry

bors · 2023-08-02T23:04:30Z

⌛ Testing commit ac25636 with merge 736ef39...

bors · 2023-08-03T00:52:52Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 736ef39 to master...

rust-timer · 2023-08-03T11:44:16Z

Finished benchmarking commit (736ef39): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-2.9%	[-3.4%, -2.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.9%	[-3.4%, -2.3%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.3%	[4.1%, 4.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 647.939s -> 648.105s (0.03%)

…ugg-in-macro, r=estebank Avoid wrong code suggesting for attribute macro Fixes rust-lang#107113 r? `@estebank`

rustbot assigned estebank Jan 24, 2023

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 Jan 24, 2023

compiler-errors reviewed Jan 24, 2023

View reviewed changes

tests/ui/coercion/coerce-block-tail-83783.stderr Show resolved Hide resolved

chenyukang commented Jan 25, 2023

View reviewed changes

chenyukang force-pushed the yukang/fix-107113-wrong-sugg-in-macro branch 2 times, most recently from b3c6813 to 5bcb80c Compare March 4, 2023 15:34

Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2023

Fix rust-lang#107113, avoid suggest for macro attributes

75b9f53

chenyukang force-pushed the yukang/fix-107113-wrong-sugg-in-macro branch from 5bcb80c to 75b9f53 Compare August 2, 2023 07:01

This comment has been minimized.

Sign in to view

fix RedundantLocals clippy caused by async and await

ac25636

chenyukang force-pushed the yukang/fix-107113-wrong-sugg-in-macro branch from 851af7f to ac25636 Compare August 2, 2023 08:33

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 2, 2023

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

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 Aug 2, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2023

bors merged commit 736ef39 into rust-lang:master Aug 3, 2023

rustbot added this to the 1.73.0 milestone Aug 3, 2023

flip1995 pushed a commit to flip1995/rust that referenced this pull request Aug 11, 2023

Auto merge of rust-lang#107254 - chenyukang:yukang/fix-107113-wrong-s…

ff27f90

…ugg-in-macro, r=estebank Avoid wrong code suggesting for attribute macro Fixes rust-lang#107113 r? `@estebank`

GrigorenkoPV mentioned this pull request Mar 28, 2024

rustc error report suggests uncompilable #[derive(PartialEq.into())] #123132

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid wrong code suggesting for attribute macro #107254

Avoid wrong code suggesting for attribute macro #107254

chenyukang commented Jan 24, 2023

m-ou-se commented Jan 24, 2023

chenyukang commented Jan 24, 2023 via email

chenyukang Jan 25, 2023

chenyukang commented Jan 25, 2023

chenyukang commented Apr 29, 2023

estebank commented May 4, 2023

chenyukang commented May 5, 2023

bors commented Jul 14, 2023

Dylan-DPC commented Aug 2, 2023

chenyukang commented Aug 2, 2023

This comment has been minimized.

rustbot commented Aug 2, 2023

chenyukang commented Aug 2, 2023

estebank commented Aug 2, 2023

bors commented Aug 2, 2023

bors commented Aug 2, 2023

bors commented Aug 2, 2023

rust-log-analyzer commented Aug 2, 2023

estebank commented Aug 2, 2023

bors commented Aug 2, 2023

bors commented Aug 3, 2023

rust-timer commented Aug 3, 2023

	pub fn can_be_used_for_suggestions(self) -> bool {
	!self.from_expansion()
	// FIXME: If this span comes from a `derive` macro but it points at code the user wrote,
	// the callsite span and the span will be pointing at different places. It also means that
	// we can safely provide suggestions on this span.
	\|\| (matches!(self.ctxt().outer_expn_data().kind, ExpnKind::Macro(MacroKind::Derive, _))
	&& self.parent_callsite().map(\|p\| (p.lo(), p.hi())) != Some((self.lo(), self.hi())))
	}

Avoid wrong code suggesting for attribute macro #107254

Avoid wrong code suggesting for attribute macro #107254

Conversation

chenyukang commented Jan 24, 2023

m-ou-se commented Jan 24, 2023

chenyukang commented Jan 24, 2023 via email

chenyukang Jan 25, 2023

Choose a reason for hiding this comment

chenyukang commented Jan 25, 2023

chenyukang commented Apr 29, 2023

estebank commented May 4, 2023

chenyukang commented May 5, 2023

bors commented Jul 14, 2023

Dylan-DPC commented Aug 2, 2023

chenyukang commented Aug 2, 2023

This comment has been minimized.

rustbot commented Aug 2, 2023

chenyukang commented Aug 2, 2023

estebank commented Aug 2, 2023

bors commented Aug 2, 2023

bors commented Aug 2, 2023

bors commented Aug 2, 2023

rust-log-analyzer commented Aug 2, 2023

estebank commented Aug 2, 2023

bors commented Aug 2, 2023

bors commented Aug 3, 2023

rust-timer commented Aug 3, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size