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

resolve/metadata: Stop encoding macros as reexports #91795

Merged
merged 4 commits into from
Feb 25, 2022

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Dec 11, 2021

Supersedes #88335.
r? @cjgillot

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2021
@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@petrochenkov petrochenkov force-pushed the nomacreexport branch 2 times, most recently from 9519fe6 to 1b37371 Compare December 12, 2021 14:31
@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 12, 2021
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum Mark-Simulacrum removed the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 12, 2021
@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 12, 2021
@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 18, 2021
hir: Do not introduce dummy type names for `extern` blocks in def paths

Use a separate nameless `DefPathData` variant instead.

Extracted from rust-lang#91795.
@petrochenkov
Copy link
Contributor Author

Blocked on #92034 and #92086.

@bors

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 2, 2022
Remove effect of `#[no_link]` attribute on name resolution

Previously it hid all non-macro names from other crates.
This has no relation to linking and can change name resolution behavior in some cases (e.g. glob conflicts), in addition to just producing the "unresolved name" errors.

I can kind of understand the possible reasoning behind the current behavior - if you can use names from a `no_link` crates then you can use, for example, functions too, but whether it will actually work or produce link-time errors will depend on random factors like inliner behavior.
(^^^ This is not the actual reason why the current behavior exist, I've looked through git history and it's mostly accidental.)

I think this risk is ok for such an obscure attribute, and we don't need to specifically prevent use of non-macro items from such crates.
(I'm not actually sure why would anyone use `#[no_link]` on a crate, even if it's macro only, if you aware of any use cases, please share. IIRC, at some point it was used for crates implementing custom derives - the now removed legacy ones, not the current proc macros.)

Extracted from rust-lang#91795.
@petrochenkov petrochenkov 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: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jan 9, 2022
@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@petrochenkov petrochenkov force-pushed the nomacreexport branch 2 times, most recently from 8d8eb69 to e2850ed Compare January 23, 2022 15:12
@bors

This comment has been minimized.

@petrochenkov
Copy link
Contributor Author

I understand that this PR resolves the ambiguity between normal exports and macro_rules re-export by adding a macro_rules flag to metadata to distinguish between the two. In that case, I'm not sure to understand what is the benefit compared to the former setup.

Previously macros were encoded as both items and reexports, but the item versions were ignored during decoding to avoid duplication.
Now they are only encoded as items are decoded as items, which is more right because macros are not reexports and it was strange to encode them as such, and also this allows to just encode less data.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Feb 4, 2022

For performance it would be optimal to use two structures:

// This struct is used during decoding and unifies both proper items and reexports
struct ModChild {
    pub ident: Ident,
    pub res: Res<!>,
    pub vis: ty::Visibility,
    pub span: Span,
    pub macro_rules: bool,
}

// This structure is used for encoding the reexport table, `macro_rules` is always false by definition
pub struct Reexport {
    pub ident: Ident,
    pub res: Res<!>,
    pub vis: ty::Visibility,
    pub span: Span,
}

, but I just didn't want to introduce more entities.
I can try it in a separate PR, but the perf benefits from not encoding this extra bool will probably be lost in noise, so it's likely not worth it.

@petrochenkov petrochenkov 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 Feb 4, 2022
@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Feb 4, 2022
@petrochenkov
Copy link
Contributor Author

Updated.

@petrochenkov petrochenkov 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 Feb 4, 2022
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@cjgillot
Copy link
Contributor

Sorry for the delay.
r=me once rebased.

…-in macros

Previously it always returned `MacroKind::Bang` while some of those macros are actually attributes and derives
To make the `macro_rules` flag more readily available without decoding everything else
@petrochenkov
Copy link
Contributor Author

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Feb 24, 2022

📌 Commit b91ec30 has been approved by cjgillot

@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 Feb 24, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2022
Rollup of 9 pull requests

Successful merges:

 - rust-lang#91795 (resolve/metadata: Stop encoding macros as reexports)
 - rust-lang#93714 (better ObligationCause for normalization errors in `can_type_implement_copy`)
 - rust-lang#94175 (Improve `--check-cfg` implementation)
 - rust-lang#94212 (Stop manually SIMDing in `swap_nonoverlapping`)
 - rust-lang#94242 (properly handle fat pointers to uninhabitable types)
 - rust-lang#94308 (Normalize main return type during mono item collection & codegen)
 - rust-lang#94315 (update auto trait lint for `PhantomData`)
 - rust-lang#94316 (Improve string literal unescaping)
 - rust-lang#94327 (Avoid emitting full macro body into JSON errors)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6ba167a into rust-lang:master Feb 25, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 25, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Nov 1, 2022
resolve: Turn the binding from `#[macro_export]` into a proper `Import`

Continuation of rust-lang#91795.

```rust
#[macro_export]
macro_rules! m { /*...*/ }
```
is desugared to something like
```rust
macro_rules! m { /*...*/ } // Non-modularized macro_rules item

pub use m; // It's modularized reexport
```

This PR adjusts the internal representation to better match this model.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

8 participants