Skip to content

Remove crate visibility modifier - #97239

Merged
bors merged 2 commits into
rust-lang:masterfrom
jhpratt:remove-crate-vis
May 21, 2022
Merged

Remove crate visibility modifier#97239
bors merged 2 commits into
rust-lang:masterfrom
jhpratt:remove-crate-vis

Conversation

@jhpratt

@jhprattjhpratt commented May 21, 2022•
edited
Loading

Copy link
Copy Markdown
Member

FCP to remove this syntax is just about complete in #53120. Once it completes, this should be merged ASAP to avoid merge conflicts.

The first two commits remove usage of the feature in this repository, while the last removes the feature itself.

@rust-highfive

rust-highfive commented May 21, 2022

Copy link
Copy Markdown
Contributor

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/rustfmt.

cc @rust-lang/rustfmt

Some changes occurred in clean/types.rs.

cc @camelid

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rustbotrustbot 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 May 21, 2022
@rust-highfive

rust-highfive commented May 21, 2022

Copy link
Copy Markdown
Contributor

r? @jsha

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2022
@jhprattjhpratt mentioned this pull request May 21, 2022
Closed
4 tasks
@jhpratt

jhpratt commented May 21, 2022

Copy link
Copy Markdown
MemberAuthor

As to how this was done: nearly every change was done via regex replacement in my editor. The only code deliberately left using this syntax is in the feature gate test.

I updated the tests and blessed the output, so they should all be up to date. The only non-trivial change was deleting a test that was self-proclaimed to be a duplicate of another. While I did manually review each part of the diff when committing, I did not do so in depth. It was merely a "yes, this diff is semantically correct" check. I presume whoever reviews this will take a similar approach.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jhprattjhpratt changed the title Remove all uses of crate visibility modifierRemove crate visibility modifierMay 21, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jackh726

jackh726 commented May 21, 2022

Copy link
Copy Markdown
Member

If you make a separate PR just removing the uses without removing the features, I'll approve that immediately. Otherwise, just wait until FCP finishes.

@jhpratt

jhpratt commented May 21, 2022

Copy link
Copy Markdown
MemberAuthor

Thanks @jackh726. I just pushed the diff that only removes the uses. If tests fail, I'll keep trying until it passes. I'm building them locally now to try to find any issues quicker.

@joshtriplett

joshtriplett commented May 21, 2022

Copy link
Copy Markdown
Member

@bors r+

@jhpratt

jhpratt commented May 21, 2022

Copy link
Copy Markdown
MemberAuthor

CI is green.

@bors

bors commented May 21, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 6970246 has been approved by joshtriplett

@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-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2022
@oli-obk

oli-obk commented May 21, 2022

Copy link
Copy Markdown
Contributor

@bors p=1 bitrotty

@bors

bors commented May 21, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 6970246 with merge 4f372b1...

@bors

bors commented May 21, 2022

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 4f372b1 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label May 21, 2022
@bors
bors merged commit 4f372b1 into rust-lang:masterMay 21, 2022
@rustbotrustbot added this to the 1.63.0 milestone May 21, 2022
@borsbors mentioned this pull request May 21, 2022
Closed
This was referenced May 21, 2022
Closed
Open
Closed
Merged
Merged
Merged
Merged
Closed
Merged
Closed
Closed
Closed
Merged
Closed
Closed
Closed
Closed
Closed
Merged
Merged
Merged
Closed
@rust-timer

rust-timer commented May 21, 2022

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (4f372b1): comparison url.

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		Regressions 😿
(secondary)		Improvements 🎉
(primary)		Improvements 🎉
(secondary)		All 😿 🎉
(primary)
count113011
mean23.2%3.3%N/A-1.1%3.2%
max3.2%5.3%N/A-1.1%3.2%

Cycles

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regression found
Regressions 😿
(primary)		Regressions 😿
(secondary)		Improvements 🎉
(primary)		Improvements 🎉
(secondary)		All 😿 🎉
(primary)
count121002
mean21.9%5.8%N/AN/A1.9%
max2.0%5.8%N/AN/A2.0%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes↩↩2

  2. the arithmetic mean of the percent change↩↩2

@jhpratt
jhpratt deleted the remove-crate-vis branch May 21, 2022 16:57
@est31est31 mentioned this pull request May 22, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2022
@bors
Auto merge ofrust-lang#97281- est31:remove_box, r=compiler-errors
b2eed72
Remove box syntax from rustc_mir_dataflow and rustc_mir_transform
Continuation of rust-lang#87781, inspired by rust-lang#97239. The usages that this PR removes have not appeared from nothing, instead the usage in `rustc_mir_dataflow` and `rustc_mir_transform` was from rust-lang#80522 which split up `rustc_mir`, and which was filed before I filed rust-lang#87781, so it was using the state from before my PR. But it was merged after my PR was merged, so the `box_syntax` uses were able to survive here. Outside of this introduction due to the code being outside of the master branch at the point of merging of my PR, there was only one other introduction of box syntax, in rust-lang#95159. That box syntax was removed again though in rust-lang#95555. Outside of that, `box_syntax` has not made its reoccurrance in compiler crates.
@dtolnaydtolnay mentioned this pull request Jan 30, 2023
Merged
@lowrlowr mentioned this pull request Jul 15, 2023
Merged
bors added a commit to rust-lang/rust-analyzer that referenced this pull request Jul 18, 2023
@bors
Auto merge of#15287- lowr:internal/remove-crate-vis, r=lnicola
3759c41
internal: remove `crate` visibility modifier
This PR removes `crate` as a now-unaccepted experimental visibility modifier from our parser. This feature has been [unaccepted] and [removed] from rustc more than a year ago, so I don't think this removal affects anyone.
[unaccepted]: rust-lang/rust#53120 (comment)
[removed]: rust-lang/rust#97239
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jshajsha

Labels

merged-by-borsThis PR was explicitly merged by bors.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.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.63.0

Development

Successfully merging this pull request may close these issues.

10 participants

@jhpratt@rust-highfive@rust-log-analyzer@jackh726@joshtriplett@bors@oli-obk@rust-timer@jsha@rustbot