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

Split out async_fn_in_trait into a separate feature #100734

Merged
merged 1 commit into from
Sep 23, 2022
Merged

Split out async_fn_in_trait into a separate feature #100734

merged 1 commit into from
Sep 23, 2022

Conversation

ComputerDruid
Copy link
Contributor

@ComputerDruid ComputerDruid commented Aug 18, 2022 •
edited
Loading

PR #101224 added support for async fn in trait desuraging behind the return_position_impl_trait_in_trait feature.

Split this out so that it's behind its own feature gate, since async fn in trait doesn't need to follow the same stabilization schedule.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nagisa (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 18, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 18, 2022
@bors
Copy link
Contributor

bors commented Aug 22, 2022

☔ The latest upstream changes (presumably #100868) made this pull request unmergeable. Please resolve the merge conflicts.

@ComputerDruid ComputerDruid force-pushed the afit_feature branch 2 times, most recently from bca9a2a to 636cabc Compare August 23, 2022 00:14
@ComputerDruid ComputerDruid marked this pull request as ready for review August 23, 2022 00:17
@nagisa
Copy link
Member

nagisa commented Aug 29, 2022

I’m not quite following what the value is behind making this change in isolation. Having a feature that simply makes certain constructs into an ICE doesn’t really help with testing the feature in any way or form, really. It also seems like a straightforward enough change that it could be introduced alongside other changes necessary to implement at least barebones support for the feature.

@tmandry
Copy link
Member

tmandry commented Aug 30, 2022

I'm working with @ComputerDruid on this and I think it's an okay place to start! In total isolation I agree landing a change like this wouldn't make sense, but an actual implementation is being worked on right now and there are a couple threads happening in parallel, so let's start by landing just the feature gate.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 30, 2022

📌 Commit 636cabc has been approved by tmandry

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 Aug 30, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 30, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 30, 2022
@matthiaskrgr
Rollup merge of rust-lang#100734 - ComputerDruid:afit_feature, r=tmandry
9966b6f 
add incomplete feature async_fn_in_trait

Doesn't do much yet, just disables the check preventing the use of the
syntax. Attempting to use it just results in ICE 🧊.
@compiler-errors
Copy link
Member

@bors r- please fix the tidy check on this, see #101182 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 30, 2022
@nagisa
Copy link
Member

nagisa commented Sep 3, 2022

r? @tmandry

@rust-highfive rust-highfive assigned tmandry and unassigned nagisa Sep 3, 2022
@ComputerDruid ComputerDruid force-pushed the afit_feature branch 2 times, most recently from ceb5b20 to d468cad Compare September 7, 2022 23:48
@ComputerDruid
Copy link
Contributor Author

@bors r- please fix the tidy check on this, see #101182 (comment)

Done.

@tmandry
Copy link
Member

tmandry commented Sep 8, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 8, 2022

📌 Commit d468cadeedd950c711ea70e02f19d21be993d386 has been approved by tmandry

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 8, 2022
@compiler-errors
Copy link
Member

compiler-errors commented Sep 9, 2022 •
edited
Loading

@tmandry, I added a return_position_impl_trait_in_trait feature in #101224 (which probably will land by tonight). Basically in the same vein as #100734 (comment) "It also seems like a straightforward enough change that it could be introduced alongside other changes necessary to implement at least barebones support for the feature" (I needed to add the gate in order to actually impl anything, lol).

Do you think we need another feature gate? I'm fine if this PR is reworked to gate AFIT desugaring behind a different feature gate I guess, but I don' t personally see a benefit. Alternatively, this PR could be reworked to rename the gate from return_position_impl_trait_in_trait to async_fn_in_trait, if there is a concern about length of the feature name.

compiler-errors
compiler-errors reviewed Sep 9, 2022
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Can you address these two comments then also add a test that checks async fn foo() -> impl Trait requires both feature gates?

@ComputerDruid
Copy link
Contributor Author

I "rebased" on #101224 . I still want to fix up a few things before this is merged:

  • update return_position_impl_trait_in_trait to use a different tracking issue
  • check some of the error messages I blessed to see if they should turn into actual test cases of the feature.

@ComputerDruid
Copy link
Contributor Author

Can you address these two comments then also add a test that checks async fn foo() -> impl Trait requires both feature gates?

I added the test, but it doesn't seem to require both features right now. Will need to debug that

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ComputerDruid
Copy link
Contributor Author

Can you address these two comments then also add a test that checks async fn foo() -> impl Trait requires both feature gates?

I added the test, but it doesn't seem to require both features right now. Will need to debug that

Should be fixed now.

@bors
Copy link
Contributor

bors commented Sep 20, 2022

☔ The latest upstream changes (presumably #102061) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

@ComputerDruid sorry, I got sick over the weekend and forgot to r+. Please rebase this and then I can approve shortly.

@ComputerDruid
Split out async_fn_in_trait into a separate feature
d0a0749 
PR rust-lang#101224 added support for async fn in trait desuraging behind the
return_position_impl_trait_in_trait feature.

Split this out so that it's behind its own feature gate, since async fn
in trait doesn't need to follow the same stabilization schedule.
@ComputerDruid
Copy link
Contributor Author

rebased and squashed into one commit

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 22, 2022

📌 Commit d0a0749 has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 22, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 22, 2022
@matthiaskrgr
Rollup merge of rust-lang#100734 - ComputerDruid:afit_feature, r=comp…
28d69ea 
…iler-errors

Split out async_fn_in_trait into a separate feature

PR rust-lang#101224 added support for async fn in trait desuraging behind the `return_position_impl_trait_in_trait` feature.

Split this out so that it's behind its own feature gate, since async fn in trait doesn't need to follow the same stabilization schedule.
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 22, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 22, 2022
@matthiaskrgr
Rollup merge of rust-lang#100734 - ComputerDruid:afit_feature, r=comp…
297f4c7 
…iler-errors

Split out async_fn_in_trait into a separate feature

PR rust-lang#101224 added support for async fn in trait desuraging behind the `return_position_impl_trait_in_trait` feature.

Split this out so that it's behind its own feature gate, since async fn in trait doesn't need to follow the same stabilization schedule.
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 22, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 23, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2022
@bors
Auto merge of rust-lang#102165 - matthiaskrgr:rollup-n5oquhe, r=matth…
4d44e09 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#100734 (Split out async_fn_in_trait into a separate feature)
 - rust-lang#101664 (Note if mismatched types have a similar name)
 - rust-lang#101815 (Migrated the rustc_passes annotation without effect diagnostic infrastructure)
 - rust-lang#102042 (Distribute rust-docs-json via rustup.)
 - rust-lang#102066 (rustdoc: remove unnecessary `max-width` on headers)
 - rust-lang#102095 (Deduplicate two functions that would soon have been three)
 - rust-lang#102104 (Set 'exec-env:RUST_BACKTRACE=0' in const-eval-select tests)
 - rust-lang#102112 (Allow full relro on powerpc64-unknown-linux-gnu)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5d7937d into rust-lang:master Sep 23, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 23, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 23, 2022
Closed
@ComputerDruid ComputerDruid deleted the afit_feature branch September 23, 2022 21:00
@tmandry tmandry mentioned this pull request Nov 1, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

Assignees

@tmandry tmandry

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.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
8 participants
@ComputerDruid @rust-highfive @bors @nagisa @tmandry @compiler-errors @rust-log-analyzer @rustbot