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

Do not provide suggestions when the spans come from expanded code that doesn't point at user code #109082

Closed
wants to merge 14 commits into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 13, 2023

Hide invalid proc-macro suggestions and track spans
coming from proc-macros pointing at attribute.

Effectively, unless the proc-macro keeps user spans,
suggestions will not be produced for the code they
produce.

r? @ghost

Fix #107113, fix #107976, fix #107977, fix #108748, fix #106720, fix #90557.

Could potentially address #50141, #67373, #55146, #78862, #74043, #88514, #83320, #91520, #104071. CC #50122, #76360.

@rustbot 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 Mar 13, 2023
@estebank estebank changed the title Do not provide suggestion to change function call when the spans come from expanded code. Do not provide suggestion to change function call when the spans come from expanded code Mar 14, 2023
@estebank estebank marked this pull request as ready for review March 14, 2023 17:42
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 14, 2023

clippy also needs some blessing

@@ -2487,6 +2487,12 @@ impl<'tcx> TyCtxt<'tcx> {
.hygienic_eq(def_name.span.ctxt(), self.expn_that_defined(def_parent_def_id))
}

pub fn adjust_span(self, span: Span) -> Span {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When should one use adjust_span? (needs some docs on this function)

All these call sites make me think that this is a bit fragile, if we forget it somewhere, a diagnostic will be subpar and it will be hard to find out where the problematic span is from. Can we do this in a more principled way? Like somewhere during ast lowering or even macro expansion?

@@ -88,6 +90,7 @@ fn e() {
//~| NOTE cannot move out of here
//~| NOTE move occurs because
//~| HELP consider borrowing here
//~| NOTE in this expansion of desugaring of a resized `Span`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this note shows up in json but not in rendered diagnostics

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'll ever want it to come up. I'll see if I can fully silence it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had some trouble finding the right place to silence this. I thought emitter.rs was the right place, but the place where this seemed to be coming from, wasn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷 not really a problem

tests/ui/feature-gates/feature-gate-concat_idents2.stderr Outdated Show resolved Hide resolved
@rustbot
Copy link
Collaborator

rustbot commented Mar 14, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@estebank
Copy link
Contributor

clippy also needs some blessing

Yeah, went ahead and fixed the lint properly in its own commit :)

@Manishearth
Copy link
Member

clippy also needs some blessing

in the name of the father, the son, and the holy @ghost, i bless thee

@rustbot
Copy link
Collaborator

rustbot commented Mar 14, 2023

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@rust-log-analyzer

This comment has been minimized.

@estebank estebank force-pushed the macro-spans branch 2 times, most recently from 05b32a6 to 4661971 Compare March 15, 2023 02:57
@estebank estebank changed the title Do not provide suggestion to change function call when the spans come from expanded code Do not provide suggestions when the spans come from expanded code that doesn't point at user code Mar 15, 2023
@estebank
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 15, 2023
@bors
Copy link
Contributor

bors commented Mar 15, 2023

⌛ Trying commit 019556d with merge a9ff946a44b0ea2531c5536d077f72f18806d291...

@bors
Copy link
Contributor

bors commented Mar 15, 2023

☀️ Try build successful - checks-actions
Build commit: a9ff946a44b0ea2531c5536d077f72f18806d291 (a9ff946a44b0ea2531c5536d077f72f18806d291)

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 15, 2023

☀️ Try build successful - checks-actions
Build commit: a9ff946a44b0ea2531c5536d077f72f18806d291 (a9ff946a44b0ea2531c5536d077f72f18806d291)

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Apr 26, 2023

The Miri subtree was changed

cc @rust-lang/miri

@bors
Copy link
Contributor

bors commented May 2, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
8 participants