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

impl Not for ! #91122

Merged
merged 4 commits into from
Jan 23, 2022
Merged

impl Not for ! #91122

merged 4 commits into from
Jan 23, 2022

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Nov 22, 2021

The lack of this impl caused trouble for me in some degenerate cases of macro-generated code of the form if !$cond {...}, even without feature(never_type) on a stable compiler. Namely if $cond contains a return or break or similar diverging expression, which would otherwise be perfectly legal in boolean position, the code previously failed to compile with:

error[E0600]: cannot apply unary operator `!` to type `!`
   --> library/core/tests/ops.rs:239:8
    |
239 |     if !return () {}
    |        ^^^^^^^^^^ cannot apply unary operator `!`

@dtolnay
Test not never
a4bff74 
Currently fails to build:

    error[E0600]: cannot apply unary operator `!` to type `!`
       --> library/core/tests/ops.rs:239:8
        |
    239 |     if !return () {}
        |        ^^^^^^^^^^ cannot apply unary operator `!`
@dtolnay
impl Not for !
9e83478
@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Nov 22, 2021
@dtolnay
Copy link
Member Author

dtolnay commented Nov 22, 2021

It looks like highfive hasn't assigned this.
r? rust-lang/libs-api

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@dtolnay dtolnay added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2021
@dtolnay dtolnay added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jan 5, 2022
@joshtriplett joshtriplett added I-lang-nominated Nominated for discussion during a lang team meeting. and removed I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Jan 5, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 5, 2022

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jan 5, 2022 •
edited by joshtriplett
Loading

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 5, 2022
@joshtriplett
Copy link
Member

joshtriplett commented Jan 5, 2022 •
edited
Loading

Nominating for the lang team because the libs-api team would like to check whether lang considers it a lang matter if we add impls of traits for !.

Note in particular that this will be visible in stable.

@scottmcm
Copy link
Member

Is this the first trait impl on ! for which there's an associated type?

Output = ! does seem reasonable for Not, but I wonder if this would be a good time to think about how to pick it more generally. Like Iterator for ! is particularly non-obvious to me.

So I don't really have any particular concern here, but this seems like it's setting a precedent for other things -- does Add for ! come next? -- where I'm not sure exactly what form it should have.

If there was going to be a lang thing here, it could be that the type checker just was smart about ! specifically. Since we might want something like impl<T> Add<!> for T and impl<T> Add<T> for !, but we can't have both of those. Maybe the answer there is that we want features for that overlap anyway?

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 11, 2022
@rfcbot
Copy link

rfcbot commented Jan 11, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 11, 2022
@joshtriplett joshtriplett removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 11, 2022
@Amanieu
Copy link
Member

Amanieu commented Jan 11, 2022

Regarding Add, can we just have a single impl Add<!> for ! and let type coercion/inference do the rest?

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 20, 2022
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 21, 2022
@rfcbot
Copy link

rfcbot commented Jan 21, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jan 21, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 22, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jan 22, 2022

📌 Commit 3136c5f has been approved by m-ou-se

@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 Jan 22, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
@matthiaskrgr
Rollup merge of rust-lang#91122 - dtolnay:not, r=m-ou-se
c8be801 
impl Not for !

The lack of this impl caused trouble for me in some degenerate cases of macro-generated code of the form `if !$cond {...}`, even without `feature(never_type)` on a stable compiler. Namely if `$cond` contains a `return` or `break` or similar diverging expression, which would otherwise be perfectly legal in boolean position, the code previously failed to compile with:

```console
error[E0600]: cannot apply unary operator `!` to type `!`
   --> library/core/tests/ops.rs:239:8
    |
239 |     if !return () {}
    |        ^^^^^^^^^^ cannot apply unary operator `!`
```
This was referenced Jan 22, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2022
@bors
Auto merge of rust-lang#93220 - matthiaskrgr:rollup-9bkrlk0, r=matthi…
16c1a9d 
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#90666 (Stabilize arc_new_cyclic)
 - rust-lang#91122 (impl Not for !)
 - rust-lang#93068 (Fix spacing for `·` between stability and source)
 - rust-lang#93103 (Tweak `expr.await` desugaring `Span`)
 - rust-lang#93113 (Unify search input and buttons size)
 - rust-lang#93168 (update uclibc instructions for new toolchain, add link from platforms doc)
 - rust-lang#93185 (rustdoc: Make some `pub` items crate-private)
 - rust-lang#93196 (Remove dead code from build_helper)

Failed merges:

 - rust-lang#93188 (rustdoc: fix bump down typing search on Safari)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 55a1f8b into rust-lang:master Jan 23, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 23, 2022
@dtolnay dtolnay deleted the not branch January 29, 2022 06:34
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 left review comments

Assignees

@joshtriplett joshtriplett

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.