Skip to content

don't panic on extern with just multiple quotes in the name - #147413

Merged
bors merged 1 commit into
rust-lang:masterfrom
karolzwolak:extern-multiple-quotes
Oct 7, 2025
Merged

don't panic on extern with just multiple quotes in the name#147413
bors merged 1 commit into
rust-lang:masterfrom
karolzwolak:extern-multiple-quotes

Conversation

@karolzwolak

@karolzwolakkarolzwolak commented Oct 6, 2025

Copy link
Copy Markdown
Member

Continues #147377.
That PR fixed ICE when the extern name was a single quote "'", but multiple quotes like "''" cause the same problem.
I had a random revelation that the trimming can remove more than one quote.
r? @nnethercote

@rustbotrustbot 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 Oct 6, 2025
/// If called on an empty ident, or with name just single quotes, returns an empty ident which is invalid.
/// Creating empty ident will trigger debug assertions.
/// Use `without_first_quote_checked` instead if not certain this will return valid ident.
pub fn without_first_quote(self) -> Ident {

@fmeasefmeaseOct 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This function is called “without first quote”, it's meant to only remove the first quote. Good catch regarding trim_start_matches (I hope nobody in the compiler is "relying" on this behavior); so please replace it with strip_prefix if possible (see suggestion below)

Comment threadcompiler/rustc_span/src/symbol.rs Outdated
/// Creating empty ident will trigger debug assertions.
/// Use `without_first_quote_checked` instead if not certain this will return valid ident.
pub fn without_first_quote(self) -> Ident {
Ident::new(Symbol::intern(self.as_str().trim_start_matches('\'')), self.span)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Ident::new(Symbol::intern(self.as_str().trim_start_matches('\'')),self.span)
self.as_str().strip_prefix('\'').map_or(self, |name| Ident::new(Symbol::intern(name),self.span))

similarly for the newly introduced function if it's even needed (I've only skimmed this PR).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, you're right I forgot that the function is without_first_quote. Let's verify that this doesn't introduce any weird regression as you mentioned though.

@fmeasefmease 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 Oct 6, 2025
@karolzwolak
karolzwolakforce-pushed the extern-multiple-quotes branch from c8d0983 to cb06d91CompareOctober 6, 2025 20:19
@karolzwolak

karolzwolak commented Oct 6, 2025

Copy link
Copy Markdown
MemberAuthor

Now the Ident::without_first_quote() will strip only the first quote -- not all leading quotes. It might be prudent to run some additional tests to make sure it doesn't break anything. I don't have the privileges nor the knowledge what try bot runs we could do though.

@fmease

fmease commented Oct 6, 2025

Copy link
Copy Markdown
Member

CI should be sufficient, it runs most tests we're interested in for this sort of change.

@fmease

Copy link
Copy Markdown
Member

Thanks for catching this! r? fmease @bors r+ rollup

@bors

bors commented Oct 6, 2025

Copy link
Copy Markdown
Collaborator

📌 Commit cb06d91 has been approved by fmease

It is now in the queue for this repository.

@borsbors 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 Oct 6, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 7, 2025
…, r=fmease
don't panic on extern with just multiple quotes in the name
Continues rust-lang#147377.
That PR fixed ICE when the extern name was a single quote `"'"`, but multiple quotes like `"''"` cause the same problem.
I had a random revelation that the trimming can remove more than one quote.
r? `@nnethercote`
This was referenced Oct 7, 2025
bors added a commit that referenced this pull request Oct 7, 2025
Rollup of 7 pull requests
Successful merges:
- #145495 (Use declarative macro for `#[derive(TryFromU32)]`)
- #147165 (test: Subtract code_offset from width for ui_testing)
- #147354 (Fix wrong span for hightlight for duplicated diff lines)
- #147395 (Improve diagnostics: update note and add help message)
- #147396 (Fluent tidy improvements)
- #147407 (Update books)
- #147413 (don't panic on extern with just multiple quotes in the name)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 21bf6db into rust-lang:masterOct 7, 2025
10 checks passed
rust-timer added a commit that referenced this pull request Oct 7, 2025
Rollup merge of #147413 - karolzwolak:extern-multiple-quotes, r=fmease
don't panic on extern with just multiple quotes in the name
Continues #147377.
That PR fixed ICE when the extern name was a single quote `"'"`, but multiple quotes like `"''"` cause the same problem.
I had a random revelation that the trimming can remove more than one quote.
r? ``@nnethercote``
@rustbotrustbot added this to the 1.92.0 milestone Oct 7, 2025
github-actionsBot pushed a commit to rust-lang/miri that referenced this pull request Oct 8, 2025
Rollup of 7 pull requests
Successful merges:
- rust-lang/rust#145495 (Use declarative macro for `#[derive(TryFromU32)]`)
- rust-lang/rust#147165 (test: Subtract code_offset from width for ui_testing)
- rust-lang/rust#147354 (Fix wrong span for hightlight for duplicated diff lines)
- rust-lang/rust#147395 (Improve diagnostics: update note and add help message)
- rust-lang/rust#147396 (Fluent tidy improvements)
- rust-lang/rust#147407 (Update books)
- rust-lang/rust#147413 (don't panic on extern with just multiple quotes in the name)
r? `@ghost`
`@rustbot` modify labels: rollup
@karolzwolak
karolzwolak deleted the extern-multiple-quotes branch October 10, 2025 18:47
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Oct 18, 2025
Rollup of 7 pull requests
Successful merges:
- rust-lang/rust#145495 (Use declarative macro for `#[derive(TryFromU32)]`)
- rust-lang/rust#147165 (test: Subtract code_offset from width for ui_testing)
- rust-lang/rust#147354 (Fix wrong span for hightlight for duplicated diff lines)
- rust-lang/rust#147395 (Improve diagnostics: update note and add help message)
- rust-lang/rust#147396 (Fluent tidy improvements)
- rust-lang/rust#147407 (Update books)
- rust-lang/rust#147413 (don't panic on extern with just multiple quotes in the name)
r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@karolzwolak@fmease@bors@nnethercote@rustbot