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.

Sign up for GitHub

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

Jump to bottom

hir: Do not introduce dummy type names for extern blocks in def paths #92032

Merged
merged 1 commit into from
Dec 18, 2021
Merged

hir: Do not introduce dummy type names for extern blocks in def paths #92032

merged 1 commit into from
Dec 18, 2021

Conversation

petrochenkov
Copy link
Contributor

Use a separate nameless DefPathData variant instead.

Extracted from #91795.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 17, 2021
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Dec 17, 2021

Is this testable on master?

@petrochenkov
Copy link
Contributor Author

This should be a pure refactoring (maybe with exception of some debug logging where {{extern}} can be printed), so no additional testing should be necessary?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 17, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 17, 2021

📌 Commit 66e232fcb27478ce9f0f404dfc24b09a8426cef0 has been approved by oli-obk

@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 Dec 17, 2021
petrochenkov
petrochenkov commented Dec 17, 2021
View reviewed changes
compiler/rustc_middle/src/ty/print/pretty.rs
// Skip `::{{constructor}}` on tuple/unit structs.
if let DefPathData::Ctor = disambiguated_data.data {
// Skip `::{{extern}}` blocks and `::{{constructor}}` on tuple/unit structs.
if let DefPathData::ForeignMod | DefPathData::Ctor = disambiguated_data.data {
return Ok(self);
}

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 forgot to remove the FIXME a couple of lines below, will fix tomorrow.

@petrochenkov
Copy link
Contributor Author

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 17, 2021
camelid
camelid reviewed Dec 17, 2021
View reviewed changes
src/librustdoc/clean/inline.rs
Comment on lines +169 to +170
// Filter out extern blocks
(elem.data != DefPathData::ForeignMod).then(|| elem.data.to_string())
Copy link
Member

Choose a reason for hiding this comment

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

cc #91948: How does this interact with that PR, specifically #91948 (comment)?

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 both PRs are merged this will turn into just elem.data.get_opt_name().

@camelid camelid mentioned this pull request Dec 17, 2021
Merged
@petrochenkov
hir: Do not introduce dummy type names for extern blocks in def paths
0d61852 
Use a separate nameless `DefPathData` variant instead
@petrochenkov
Copy link
Contributor Author

Updated with #92032 (comment) addressed.
@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Dec 18, 2021

📌 Commit 0d61852 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 18, 2021
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 18, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2021
@bors
Auto merge of rust-lang#92064 - matthiaskrgr:rollup-tgj2pai, r=matthi…
d3848cb 
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91858 (pass -Wl,-z,origin to set DF_ORIGIN when using rpath)
 - rust-lang#91923 (Remove `in_band_lifetimes` from `rustc_query_impl`)
 - rust-lang#91925 (Remove `in_band_lifetimes` from `rustc_privacy`)
 - rust-lang#91977 (Clean up search code and unify function returned values)
 - rust-lang#92018 (Fix typo in "new region bound" suggestion)
 - rust-lang#92022 (Eliminate duplicate codes of expected_found_bool)
 - rust-lang#92032 (hir: Do not introduce dummy type names for `extern` blocks in def paths)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Aaron1011
Aaron1011 reviewed Dec 18, 2021
View reviewed changes
compiler/rustc_symbol_mangling/src/v0.rs
@@ -771,6 +771,10 @@ impl<'tcx> Printer<'tcx> for &mut SymbolMangler<'tcx> {
disambiguated_data: &DisambiguatedDefPathData,
) -> Result<Self::Path, Self::Error> {
let ns = match disambiguated_data.data {
// FIXME: It shouldn't be necessary to add anything for extern block segments,
// but we add 't' for backward compatibility.
DefPathData::ForeignMod => 't',
Copy link
Member

Choose a reason for hiding this comment

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

Is this code actually reachable? What will the demangled symbol look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's reachable.
Functions and statics in foreign blocks are not mangled, but foreign types are mangled when they are used in any non-foreign symbols.
Something like

extern { type Type; }
fn foo(_arg: &Type) {}

@bors bors merged commit 5e8f934 into rust-lang:master Dec 18, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 18, 2021
@petrochenkov petrochenkov mentioned this pull request Dec 27, 2021
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 8, 2022
@matthiaskrgr
Rollup merge of rust-lang#92316 - petrochenkov:extmangle, r=wesleywiser
f3e9260 
mangling_v0: Skip extern blocks during mangling

There's no need to include the dummy `Nt` into the symbol name, items in extern blocks belong to their parent modules for all purposes except for inheriting the ABI and attributes.

Follow up to rust-lang#92032

(There's also a drive-by fix to the `rust-demangler` tool's tests, which don't run on CI, I initially attempted using them for testing this PR.)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 9, 2022
@matthiaskrgr
Rollup merge of rust-lang#92316 - petrochenkov:extmangle, r=wesleywiser
b34a770 
mangling_v0: Skip extern blocks during mangling

There's no need to include the dummy `Nt` into the symbol name, items in extern blocks belong to their parent modules for all purposes except for inheriting the ABI and attributes.

Follow up to rust-lang#92032

(There's also a drive-by fix to the `rust-demangler` tool's tests, which don't run on CI, I initially attempted using them for testing this PR.)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2022
@matthiaskrgr
Rollup merge of rust-lang#92316 - petrochenkov:extmangle, r=wesleywiser
3c2fed6 
mangling_v0: Skip extern blocks during mangling

There's no need to include the dummy `Nt` into the symbol name, items in extern blocks belong to their parent modules for all purposes except for inheriting the ABI and attributes.

Follow up to rust-lang#92032

(There's also a drive-by fix to the `rust-demangler` tool's tests, which don't run on CI, I initially attempted using them for testing this PR.)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 19, 2022
@matthiaskrgr
Rollup merge of rust-lang#92316 - petrochenkov:extmangle, r=wesleywiser
715cda2 
mangling_v0: Skip extern blocks during mangling

There's no need to include the dummy `Nt` into the symbol name, items in extern blocks belong to their parent modules for all purposes except for inheriting the ABI and attributes.

Follow up to rust-lang#92032

(There's also a drive-by fix to the `rust-demangler` tool's tests, which don't run on CI, I initially attempted using them for testing this PR.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaron1011 Aaron1011 Aaron1011 left review comments

@camelid camelid camelid left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
7 participants
@petrochenkov @rust-highfive @oli-obk @bors @Aaron1011 @camelid @rustbot