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

delegation: Implement list delegation #123413

Merged
merged 1 commit into from
May 15, 2024
Merged

delegation: Implement list delegation #123413

merged 1 commit into from
May 15, 2024

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Apr 3, 2024 •
edited 

reuse prefix::{a, b, c};

Using design described in rust-lang/rfcs#3530 (comment) (the lists are desugared at macro expansion time).
List delegations are expanded eagerly when encountered, similarly to #[cfg]s, and not enqueued for later resolution/expansion like regular macros or glob delegation (#124135).

Part of #118212.

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Apr 3, 2024
@fmease
Copy link
Member

fmease commented Apr 12, 2024

Wait, this conflicts with #122500, which one should I review first?

@petrochenkov
Copy link
Contributor Author

@fmease
Any order should be fine.

@bors

This comment was marked as resolved.

Sign in to view
@petrochenkov petrochenkov mentioned this pull request Apr 18, 2024
Open
@rustbot
Copy link
Collaborator

rustbot commented Apr 19, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@petrochenkov petrochenkov changed the title delegation: Implement multi-item delegation delegation: Implement list delegation Apr 19, 2024
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Apr 19, 2024 •
edited

Based on the experience of implementing glob delegation in #124135, I changed the implementation here from using DelegationKind to using a new ItemKind for delegation lists, which is treated similarly to ItemKind::MacCall.
It requires touching more code, but the result is clearer.
(I also found missing pretty-printing this way, and fixed it.)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov force-pushed the delegmulti2 branch 2 times, most recently from bf178ca to c60d449 Compare April 24, 2024 16:33
@petrochenkov
Copy link
Contributor Author

@fmease
I rebased and changed the treatment of DelegationMac in expand.rs to be more similar to other macros.

@bors

This comment was marked as resolved.

Sign in to view
@fmease fmease added the F-fn_delegation `#![feature(fn_delegation)]` label May 14, 2024
fmease
fmease approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay 😞

r=me after rebase & with suggestions & questions addressed.

compiler/rustc_ast/src/mut_visit.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/item.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/item.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/item.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_pretty/src/pprust/state/item.rs
}
if let Some(suffixes) = suffixes {
self.word("::");
self.word("{");
Copy link
Member

Choose a reason for hiding this comment

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

(minor, not blocking) could maybe use one of ibox, cbox

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'll leave this for some later time.

compiler/rustc_ast_pretty/src/pprust/state/item.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_pretty/src/pprust/state/item.rs Outdated Show resolved Hide resolved
compiler/rustc_expand/src/expand.rs Show resolved Hide resolved
compiler/rustc_parse/src/parser/item.rs Show resolved Hide resolved
tests/ui/delegation/list.rs Outdated Show resolved Hide resolved
@fmease fmease mentioned this pull request May 14, 2024
Open
4 tasks
@fmease fmease 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 May 14, 2024
@petrochenkov
delegation: Implement list delegation
c30b410 
```rust
reuse prefix::{a, b, c}
```
@petrochenkov
Copy link
Contributor Author

@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 May 14, 2024
fmease
fmease approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks!

@fmease
Copy link
Member

fmease commented May 15, 2024

@bors r+

@bors
Copy link
Contributor

bors commented May 15, 2024

📌 Commit c30b410 has been approved by fmease

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 May 15, 2024
@bors
Copy link
Contributor

bors commented May 15, 2024

⌛ Testing commit c30b410 with merge 3cb0030...

@bors
Copy link
Contributor

bors commented May 15, 2024

☀️ Test successful - checks-actions
Approved by: fmease
Pushing 3cb0030 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 15, 2024
@bors bors merged commit 3cb0030 into rust-lang:master May 15, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 15, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3cb0030): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

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)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.4% [-0.5%, -0.4%] 2
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 2.1%)

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

Cycles

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

Binary size

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

Bootstrap: 678.182s -> 679.399s (0.18%)
Artifact size: 316.22 MiB -> 316.22 MiB (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease approved these changes

Assignees

@fmease fmease

Labels
F-fn_delegation `#![feature(fn_delegation)]` 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
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@petrochenkov @rustbot @fmease @bors @rust-log-analyzer @rust-timer