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

add the bool::not method. #82786

Closed
wants to merge 5 commits into from
Closed

add the bool::not method. #82786

wants to merge 5 commits into from

Conversation

Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Mar 5, 2021

This adds a not method to the bool type so that you can call b.not() without having to have core::ops::Not in scope. Also, a feature gate for the method.

This is a much smaller overall change compared to placing core::opt::Not into the 2021 prelude (as some have proposed).

(@jyn514 gave mild mentor advice on how to get started here and wanted a ping when it was posted)

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 5, 2021
@rust-log-analyzer

This comment has been minimized.

@Lokathor
Copy link
Contributor Author

Lokathor commented Mar 5, 2021

Well, now i need some outside advice on if i should change the test or just abandon the PR because T-libs doesn't even want this.

@osa1
Copy link
Contributor

osa1 commented Mar 6, 2021

Well, now i need some outside advice on if i should change the test or just abandon the PR because T-libs doesn't even want this.

Would be good to have the link to T-libs discussion in the PR description.

@Lokathor
Copy link
Contributor Author

Lokathor commented Mar 6, 2021

I didn't create the PR in response to any particular T-libs discussion, other than musings that some people had in the edition prelude RFC.

@the8472

This comment has been minimized.

/// assert_eq!(true.not(), false);
/// ```
#[unstable(feature = "bool_not_method", issue = "none")]
pub fn not(self) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method could be a const fn, did you decide not to make it one to keep it identical to the Not::not method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point actually. i only thought of it initially as a replacement for the Not trait being in the prelude, but we could probably go const here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tangentially, it being constable already can also be an added argument for making it an inherent method

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the intention is to make it possible to make trait impls const also, but we're pretty far from seeing that stabilized.

@rust-log-analyzer

This comment has been minimized.

@danielhenrymantilla
Copy link
Contributor

Hmm 🤔 while this method is unstable, what happens if somebody does:

use ::core::ops::Not /* as _ */;

some_bool.not()

?

Is there a way to make unstable_name_collisions not trigger on that very pattern? There is no name collision in practice, but for the Not import potentially becoming an unused_import once the method becomes stable, which is not a problem per se.

@crlf0710
Copy link
Member

Triage: What's next steps here?

@jyn514
Copy link
Member

jyn514 commented Mar 26, 2021

I think this is waiting on review from @m-ou-se.

@Lokathor
Copy link
Contributor Author

Particularly, they (and the rest of libs) need to make a call on how to handle the failing tests. The code is not incorrect, rather the failing tests were written specifically against this situation. So either I'm told to change the tests or we abandon the PR entirely.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 27, 2021

Hey @rust-lang/libs, do any of you have an opinion on this addition?

@BurntSushi
Copy link
Member

Personally, I'm ambivalent. One part of me doesn't want not to become frictionless to use, because I'm not a huge fan of the idea of having both not() and ! be in widespread use. On the other hand, the not method is already a thing. The only thing preventing folks from using it is some minor friction related to having to write an import. Another way to look at it is, not() has not been considered a strong enough improvement to readability for most folks to justify the extra import. If that's the case, then is it really worth it?

But, there is something to be said for reasonable people to disagree over which is more readable or not. And I doubt there will be much confusion between uses of ! and not(), other than the, "is there any difference other than style/readability?" type questions. It does kind of strike me as that quintessential newbie question that asks them to make a decision on which to use because they might see code out in the wild using either one. But maybe the solution to that is to add a bit more docs to this method that says it is "Equivalent to using ! on a boolean value. The use of this method is an aesthetic preference."

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Mar 27, 2021

Aside: a personal example of a preference for suffix negation

So I personally like the .not() more, because it works better with method chaining for the case of long conditions, and then for personal consistency with short conditions. The latter is where more subjectivity will be at play: I can definitely understand that most people can prefer if !empty { over if empty.not() {, although to me both of these are equivalent, and the consistency argument being the one tiping the balance in favor of the suffix notation.

  • The main drawback against currently using .not is the churn of requiring that Not be in scope, which thus leads to a circular situation against this pattern 🙁

That being said, I do think that since the language chose prefix ! to begin with, se should never try to hinder that notation, and I think that most supporters of a postfix notation agree with this: we fully respect those who prefer the prefix sigil; we just ask for the same kind of respect "back" 😄


One way to do things?

I have personally often been skeptical of following the "there should be only one way to do things" idea too blindly, since in practice applying that policy too harshly makes that utopia quickly become a chimera: the natural evolution of a language is likely to end up offering alternative ways to express the same idea, and while feature creeping is a thing, stagnation is also another.

  • If we take Python, for instance, a language which did publicly showcase the "there should be only one way to do things" philosophy, they did not always follow it. For instance, they started diverging from the C-looking string formatting to even end up supporting fstrings formatting, since there were benefits to doing so (fstrings are convenient for short / trivial format strings, but regular formatting is better when more complex expressions are involved).

  • Back to Rust, we have several ways to convert a &str to a String, several ways to perform by-ref pattern-matching, several ways to launder the lifetime of a borrow to dodge a borrowck false negative; we have .into() and From::from, we have .collect() and from_iter, we have for x in &slice and for x in slice.iter(), etc.

Natural languages do feature synonyms, and the context often suggests which ones are more accurate / adapted to the situation than others, and I think something similar applies to programming languages, no matter how much we may want a unified programming style 🙂

@BurntSushi
Copy link
Member

@danielhenrymantilla Just to make a small clarification: I specifically did not use the general language of "there should only be one way to do things." I am of course aware of the fact that Rust has never followed that philosophy generally and that Python has largely failed to do so despite it being a design goal. :-) But rather, I am expressing a more narrow opinion on this specific case.

@joshtriplett
Copy link
Member

Mild support. I also hope we have an "inherent traits" mechanism in the future, in which case I'd be generally supportive of marking various traits inherent.

@sfackler
Copy link
Member

I am personally not a fan of adding ad-hoc copes of random methods just to avoid an import.

@Lokathor
Copy link
Contributor Author

@sfackler This PR exists in the context of the Prelude changes arguments. By simply adding the one not method to the one type that really needs it, we can effectively remove need to debate adding the Not trait to the 2018 prelude. In the process, we help 2015 users as well.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-10 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.................................................................................................... 9400/11718
.................................................................................................... 9500/11718
............................................................i......i................................ 9600/11718
.................................................................................................... 9700/11718
......iiiiiii..iiiiii.i............................................................................. 9800/11718
.................................................................................................... 10000/11718
.................................................................................................... 10100/11718
.................................................................................................... 10200/11718
.................................................................................................... 10300/11718
---
normalized stderr:
warning: an associated function with this name may be added to the standard library in the future
  --> $DIR/macro-expanded.rs:20:19
   |
LL |             false.not();
...
...
LL | m!();
   |
   = note: `#[warn(unstable_name_collisions)]` on by default
   = note: `#[warn(unstable_name_collisions)]` on by default
   = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
   = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
   = help: call with fully qualified syntax `not(...)` to keep using the current method
   = help: add `#![feature(bool_not_method)]` to the crate attributes to enable `core::bool::<impl bool>::not`

warning: an associated function with this name may be added to the standard library in the future
  --> $DIR/macro-expanded.rs:36:15
   |
   |
LL |         false.not();
...
...
LL | n!();
   |
   |
   = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
   = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
   = help: call with fully qualified syntax `not(...)` to keep using the current method
   = help: add `#![feature(bool_not_method)]` to the crate attributes to enable `core::bool::<impl bool>::not`
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu

warning: 2 warnings emitted





The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/underscore-imports/macro-expanded/macro-expanded.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args underscore-imports/macro-expanded.rs`
error: 1 errors occurred comparing output.
status: exit status: 0
status: exit status: 0
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/underscore-imports/macro-expanded.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zemit-future-incompat-report" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/underscore-imports/macro-expanded" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/underscore-imports/macro-expanded/auxiliary"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
warning: an associated function with this name may be added to the standard library in the future
  --> /checkout/src/test/ui/underscore-imports/macro-expanded.rs:20:19
   |
LL |             false.not();
...
...
LL | m!();
   |
   = note: `#[warn(unstable_name_collisions)]` on by default
   = note: `#[warn(unstable_name_collisions)]` on by default
   = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
   = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
   = help: call with fully qualified syntax `not(...)` to keep using the current method
   = help: add `#![feature(bool_not_method)]` to the crate attributes to enable `core::bool::<impl bool>::not`

warning: an associated function with this name may be added to the standard library in the future
  --> /checkout/src/test/ui/underscore-imports/macro-expanded.rs:36:15
   |
   |
LL |         false.not();
...
...
LL | n!();
   |
   |
   = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
   = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
   = help: call with fully qualified syntax `not(...)` to keep using the current method
   = help: add `#![feature(bool_not_method)]` to the crate attributes to enable `core::bool::<impl bool>::not`

warning: 2 warnings emitted


---
test result: FAILED. 11621 passed; 1 failed; 96 ignored; 0 measured; 0 filtered out; finished in 134.70s



command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--suite" "ui" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-10/bin/FileCheck" "--nodejs" "/usr/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "10.0.0" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets amdgpu amdgpuasmparser amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgpuutils analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker engine executionengine frontendopenmp fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interpreter ipo irreader jitlink lanai lanaiasmparser lanaicodegen lanaidesc lanaidisassembler lanaiinfo libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcerror orcjit passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo riscvutils runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info x86utils xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --exclude src/tools/tidy
Build completed unsuccessfully in 0:12:46

@Dylan-DPC-zz
Copy link

@m-ou-se any updates?

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs meeting. We realized that if we add this, it'll start generating an "unused import" warning for everyone currently importing core::ops::Not for this. For that reason, we'd like to close this, in favor of one of the "inherent traits" proposals that would allow calling .not() on bool without having to import core::ops::Not; we'd expect that those proposals would not generate an unused import warning.

@joshtriplett
Copy link
Member

@rfcbot close

@rfcbot
Copy link

rfcbot commented Apr 14, 2021

Team member @joshtriplett has proposed to close 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-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Apr 14, 2021
@Lokathor Lokathor closed this Apr 14, 2021
@Lokathor
Copy link
Contributor Author

I'll just close it then, the rfc check boxes seem like a needless formality if it's not going to be merged.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Apr 14, 2021
@Lokathor Lokathor deleted the bool-not branch April 14, 2021 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.