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

Properly handle lint spans after MIR inlining #76931

Merged
merged 3 commits into from
Nov 3, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Sep 19, 2020

The first commit shows what happens when we apply mir inlining and then cause lints on the inlined MIR.
The second commit fixes that.

r? @wesleywiser

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 19, 2020

cc @lcnr

src/test/ui/const_prop/inline_spans.stderr Outdated Show resolved Hide resolved
fn visit_span(&mut self, span: &mut Span) {
*span = self.callsite_span.fresh_expansion(ExpnData {
def_site: self.body_span,
..ExpnData::default(ExpnKind::Inlined, *span, self.tcx.sess.edition(), None)
Copy link
Member

Choose a reason for hiding this comment

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

Should the edition be the edition of span? We could be inlining something from another crate which has a different edition than our current one.

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 was wondering that, too, but I don't see how the edition is relevant from the MIR on anyway, all of that checking happens waay earlier. I can't extract the edition from the span, because it may not have any ExpnData, so I would have to find the crate of the Span and check its edition. Doing something like this from a Span is something I don't think we've done before

@eddyb

This comment has been minimized.

@oli-obk oli-obk added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 20, 2020
@camelid camelid removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 16, 2020
@camelid

This comment has been minimized.

@oli-obk oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 27, 2020
@oli-obk oli-obk force-pushed the const_prop_inline_lint_madness branch from 4998709 to c8a866e Compare October 27, 2020 14:08
@oli-obk oli-obk added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 27, 2020
@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 3, 2020

📌 Commit 888ef24 has been approved by wesleywiser

@bors 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 Nov 3, 2020
@@ -56,7 +56,7 @@
StorageLive(_6); // scope 3 at $DIR/cycle.rs:14:10: 14:11
- _6 = _1; // scope 3 at $DIR/cycle.rs:14:10: 14:11
+ _6 = _4; // scope 3 at $DIR/cycle.rs:14:10: 14:11
_5 = const (); // scope 4 at $SRC_DIR/core/src/mem/mod.rs:LL:COL
_5 = const (); // scope 4 at $DIR/cycle.rs:14:5: 14:12
Copy link
Member

Choose a reason for hiding this comment

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

Does this affect the generated debuginfo? LLVM inlining preserves the original source position instead of using the position of the inlined call. The MIR inliner should do the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The debuginfo part was handled in #68965

This here just addresses spans for diagnostics. These spans do preserve the original and all intermediate source positions across any level of inlining. I should probably adjust mir dumps to also print all spans instead of just the final one.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 3, 2020
…ess, r=wesleywiser

Properly handle lint spans after MIR inlining

The first commit shows what happens when we apply mir inlining and then cause lints on the inlined MIR.
The second commit fixes that.

r? `@wesleywiser`
@bors
Copy link
Contributor

bors commented Nov 3, 2020

⌛ Testing commit 888ef24 with merge 5cdf5b8...

@bors
Copy link
Contributor

bors commented Nov 3, 2020

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 5cdf5b8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 3, 2020
@bors bors merged commit 5cdf5b8 into rust-lang:master Nov 3, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 3, 2020
@oli-obk oli-obk deleted the const_prop_inline_lint_madness branch March 16, 2021 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants