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

Add debug assertions for empty replacements and overlapping spans #13567

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

y21
Copy link
Member

@y21 y21 commented Oct 19, 2024

rustc has debug assertions 1 2 that check that a substitution doesn't have an empty suggestion string and an empty span at the same time, as well as that spans in multipart suggestions don't overlap.
However, since we link to the rustc-dev distributed compiler, these debug assertions are always disabled and so we never actually run them.

This leads to the problem that the debug ICE is not necessarily caught in the PR and only triggered in the rust repo sync, and in one of the last syncs this was a blocker and delayed the sync by several weeks because the fix was not obvious.

So this PR essentially copies the checks over and runs them in clippy debug builds as well, so that we can catch these errors in PRs directly.


As for the second commit, this also did cause an ICE in a sync before and was fixed in the sync PR (see rust-lang/rust#120345 (comment)), but it seems like that commit didn't make it back into the clippy repo (cc @flip1995), so the fixed code is in the rust repo but not in the clippy repo.

changelog: none

Footnotes

  1. https://doc.rust-lang.org/1.82.0/nightly-rustc/src/rustc_errors/diagnostic.rs.html#1019

  2. https://doc.rust-lang.org/1.82.0/nightly-rustc/src/rustc_errors/diagnostic.rs.html#932

@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2024

r? @Centri3

rustbot has assigned @Centri3.
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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 19, 2024
@y21 y21 force-pushed the span_debug_assertions branch from de84b25 to 4de65a1 Compare October 19, 2024 17:25
@y21
Copy link
Member Author

y21 commented Oct 20, 2024

Looks like lintcheck with this assertion unconvered another bug. TIL that the span of a macro call semicolon statement m!(); doesn't actually include the semicolon and instead the statement has the same span as the expression (leading to a debug ICE in statement_outside_block for unsafe { asm!(""); } as trying to remove the semicolon creates an empty replacement).

…outside_block`

The expansion of `asm!()` and `line!()` is not marked as from an expansion, in which case `SourceMap::stmt_span` returns the input span unchanged. So instead of using `stmt_span`, use `mac_call_stmt_semi_span` directly
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Thanks for making my life easier!

let remove_span = semi_span.with_lo(tail_stmt_expr.span.source_callsite().hi());

// For macro call semicolon statements (`mac!();`), the statement's span does not actually
// include the semicolon itself, so use `mac_call_stmt_semi_span`, which finds the semicolon
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that this function exists!

@@ -181,9 +181,6 @@ fn convert_to_from(
let from = self_ty.span.get_source_text(cx)?;
let into = target_ty.span.get_source_text(cx)?;

let return_type = matches!(sig.decl.output, FnRetTy::Return(_))
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that this didn't get synced (together with a few other things) when doing the Josh setup. With Josh all of that would be synced. The problem with subtree is, that if you mess up a merge once, it will never be synced correctly again. Luckily this didn't happen that often yet and will get fixed, once we move to Josh.

This is a commit fixing everything that wasn't correctly synced with subtree: flip1995@89f1145

@flip1995
Copy link
Member

r? @flip1995

@flip1995
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 21, 2024

📌 Commit 65eb1ec has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 21, 2024

⌛ Testing commit 65eb1ec with merge 8538562...

@bors
Copy link
Contributor

bors commented Oct 21, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 8538562 to master...

@bors bors merged commit 8538562 into rust-lang:master Oct 21, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants