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: mark binding is determined after all macros had been expanded #115269

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Aug 27, 2023

Fixes #113834
Fixes #115377

r? @petrochenkov

@rustbot rustbot 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 Aug 27, 2023
@rust-log-analyzer

This comment has been minimized.

@bvanjoi bvanjoi marked this pull request as draft August 27, 2023 12:02
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Aug 27, 2023

ci is failed, thus mark it as draft

@rust-log-analyzer

This comment has been minimized.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Aug 28, 2023

reduced for #115269 (comment):

// compile-flags: --edition 2021
// check-pass

macro_rules! types {
    () => {};
}

mod x {
    types!();

    mod a {
        macro_rules! m {
            () => {
                "nop"
            };
        }
        pub(crate) use m;
    }

    mod b {
        use crate::x::*;
        use std::arch::asm;

        pub unsafe fn bbbb() {
            asm!(m!());
        }
    }

    pub use self::a::*;
    pub use self::b::*;
}

fn main() {}

compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/imports.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

I still think it would be nice to make some refactorings mentioned in https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Is.20this.20an.20appropriate.20solution.20for.20issue.20.23113834.3F/near/386775859 before proceeding to further fixes.
(Changing glob_importers to a set can also be included into refactorings.)

@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 Aug 30, 2023
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Sep 3, 2023

I have updated the algorithm as follows: the glob binding is marked as undetermined if there are unexpended macros in its parent module. Then, it is marked as determined once all macros have been expanded. This flag can be used to update the resolution for it.

I think a good refactoring would be factor out this logic and just call something like update_resolution from resolve_glob_import.

I'm not entirely sure if this is what you're seeking, but I think it comes close.

@rustbot ready

@rustbot rustbot 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 Sep 3, 2023
@bvanjoi bvanjoi marked this pull request as ready for review September 3, 2023 04:11
@bvanjoi bvanjoi changed the title resolve: delay resolve glob import when it has invocation macros resolve: mark binding is determined when all macros is expanded in the parent module Sep 3, 2023
@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 Sep 4, 2023
compiler/rustc_resolve/src/ident.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/imports.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

@bors try

@petrochenkov
Copy link
Contributor

@bors try

@bvanjoi bvanjoi changed the title resolve: mark binding is determined when all macros is expanded in the parent module resolve: mark binding is determined after all macros had been expanded Sep 12, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Sep 12, 2023

I've added a pre-condition: xxx && import.is_glob() {}

@rustbot ready

@rustbot rustbot 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 Sep 12, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 13, 2023

📌 Commit 9ea0bbd has been approved by petrochenkov

It is now in the queue for this repository.

@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 Sep 13, 2023
@bors
Copy link
Contributor

bors commented Sep 13, 2023

⌛ Testing commit 9ea0bbd with merge c5d798c...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
resolve: mark binding is determined after all macros had been expanded

Fixes rust-lang#113834
Fixes rust-lang#115377

r? `@petrochenkov`
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 13, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 13, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Sep 13, 2023

I removed this test in #115269 (comment) as there has a similar struct on stdarch

@rustbot ready

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 13, 2023

📌 Commit f153650 has been approved by petrochenkov

It is now in the queue for this repository.

@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 Sep 13, 2023
@bors
Copy link
Contributor

bors commented Sep 13, 2023

⌛ Testing commit f153650 with merge 735bb7e...

@bors
Copy link
Contributor

bors commented Sep 13, 2023

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 735bb7e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 13, 2023
@bors bors merged commit 735bb7e into rust-lang:master Sep 13, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 13, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (735bb7e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.2% [1.5%, 3.4%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-3.1%, -2.8%] 2
All ❌✅ (primary) 2.2% [1.5%, 3.4%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.8% [3.8%, 3.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 633.201s -> 632.169s (-0.16%)
Artifact size: 318.02 MiB -> 317.92 MiB (-0.03%)

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants