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

fix(resolve): update the ambiguity glob binding as warning recursively #113099

Merged
merged 2 commits into from
Jul 29, 2023

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Jun 27, 2023

Fixes #47525
Fixes #56593, but issue-56593-2.rs is not fixed to ensure backward compatibility.
Fixes #98467
Fixes #105235
Fixes #112713

This PR had added a field called warn_ambiguous in NameBinding which is only for back compatibly reason and used for lint.

More details: #112743

r? @petrochenkov

@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@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 Jun 27, 2023
@rustbot rustbot assigned petrochenkov and unassigned TaKO8Ki Jun 27, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Meta: not sure why new PR, we could continue in #112743, but it's fine too.

compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
tests/ui/resolve/issue-56593-2.rs Outdated Show resolved Hide resolved
tests/ui/resolve/issue-56593-2.rs Outdated Show resolved Hide resolved
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jun 29, 2023

not sure why new PR

Ahh, just because I feel there are some differences in the way they are implemented, perhaps the new PR can provide a clearer message. Of course, this matter is not important, and we don't need to dwell on it.

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_lint_defs/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/context.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/imports.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

Could you also move all the new tests from ui/resolve to ui/imports?

@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 Jun 29, 2023
@bors

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@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 Jul 1, 2023
compiler/rustc_resolve/src/imports.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/diagnostics.rs Outdated Show resolved Hide resolved
@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 Jul 1, 2023
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 26, 2023
@bors

This comment was marked as resolved.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jul 28, 2023

I have added #[allow(ambiguous_glob_reexports)] f mod core_arch.

@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 Jul 28, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 28, 2023

📌 Commit 771c832 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 Jul 28, 2023
@bors
Copy link
Contributor

bors commented Jul 29, 2023

⌛ Testing commit 771c832 with merge 2dc6610...

@bors
Copy link
Contributor

bors commented Jul 29, 2023

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 2dc6610 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 29, 2023
@bors bors merged commit 2dc6610 into rust-lang:master Jul 29, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 29, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jul 29, 2023

As the next step, I will attempt to address the issue at #113834.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2dc6610): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.5% [1.2%, 2.6%] 13
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.3% [-0.3%, -0.3%] 1
All ❌✅ (primary) - - 0

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)
1.7% [1.1%, 2.3%] 6
Regressions ❌
(secondary)
7.8% [6.2%, 9.7%] 7
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.0% [-4.1%, -2.4%] 3
All ❌✅ (primary) 1.7% [1.1%, 2.3%] 6

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)
2.6% [2.1%, 3.5%] 8
Improvements ✅
(primary)
-2.3% [-2.3%, -2.3%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.3% [-2.3%, -2.3%] 1

Binary size

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

Bootstrap: 650.158s -> 652.195s (0.31%)

@rustbot rustbot added the perf-regression Performance regression. label Jul 29, 2023
@nnethercote
Copy link
Contributor

Minor regressions for unused-warnings, but that is an extreme stress tests that exists mostly to detect catastrophic regressions in error handling, so it's no big deal.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 30, 2023
@tmiasko tmiasko added beta-nominated Nominated for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Sep 7, 2023
Warn,
"detects certain glob imports that require reporting an ambiguity error",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseError,
Copy link
Member

Choose a reason for hiding this comment

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

FWIW this FutureIncompatibilityReason will make the warning only show up in locally built crates, but not in cargo's future breakage report that also shows warnings from dependencies. Just making sure that this is a deliberate choice and not an accident. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I just assumed all future incompatibility lints are reported for dependencies during review.

Copy link
Member

Choose a reason for hiding this comment

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

No, that's not the case. #116049 should help clarify (but doesn't change the behavior of any existing lint).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet