Page MenuHomePhabricator

[libcxx] simplifies nodiscard by permanently enabling the attribute...
Needs RevisionPublic

Authored by cjdb on Aug 14 2021, 3:20 PM.

Details

Reviewers
ldionne
zoecarver
Mordante
Group Reviewers
Restricted Project
Summary

...for anything that already had _LIBCPP_NODISCARD_EXT or
_LIBCPP_NODISCARD_AFTER_CXX17.

nodiscard in libc++ has had multi-layered complexity for quite some
time, requiring users to know that they need to opt into a lot of what
we mark as nodiscard. There are also many knobs: an opt-in mechanism
(that users can easily overlook) and three opt-out mechanisms, which can
lead to confused users.

This patch does not break any code that wasn't already buggy or
questionable to begin with.

Diff Detail

Event Timeline

cjdb requested review of this revision.Aug 14 2021, 3:20 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2021, 3:20 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/docs/UsingLibcxx.rst
274–281

This PR seems acceptable to me. (I think it's likely better than the status quo, because it's simpler.) However, I'd still prefer to see libc++ distinguish the "conforming implementation" parts from the "non-conforming" parts. After all, this is supposed to be a valid C++ program, right?

#include <utility>
int main() { std::move(1); }

With this PR, libc++ will refuse to compile that program, so libc++ won't technically be conforming anymore. This could piss off customers with big old codebases. Whenever we do an API-breaking change, it's useful to give customers a way to opt out of it.

So, I counterpropose that we still do most of this simplification, but:

  • Continue to mark the extensions with _LIBCPP_NODISCARD_EXT instead of _LIBCPP_NODISCARD
  • Make __config look like this:
#if __has_cpp_attribute(nodiscard) || defined(_LIBCPP_COMPILER_MSVC)
#  define _LIBCPP_NODISCARD [[nodiscard]]
#elif defined(_LIBCPP_COMPILER_CLANG_BASED) && !defined(_LIBCPP_CXX03_LANG)
#  define _LIBCPP_NODISCARD [[clang::warn_unused_result]]
#else
// We can't use GCC's [[gnu::warn_unused_result]] and
// __attribute__((warn_unused_result)), because GCC does not silence them via
// (void) cast.
#  define _LIBCPP_NODISCARD
#endif

#if defined(_LIBCPP_DISABLE_NODISCARD_EXT) && _LIBCPP_DISABLE_NODISCARD_EXT
#  define _LIBCPP_NODISCARD_EXT
#else
#  define _LIBCPP_NODISCARD_EXT _LIBCPP_NODISCARD
#endif
cjdb added inline comments.Aug 15 2021, 9:15 AM
libcxx/docs/UsingLibcxx.rst
274–281

After all, this is supposed to be a valid C++ program, right?

#include <utility>
int main() { std::move(1); }

This is also a conforming C++20 program.

#include <vector>

int main()
{
  auto v = std::vector{0, 1, 2};
  v.empty(); // [[nodiscard]] as of C++20
}

With this PR, libc++ will refuse to compile that program, so libc++ won't technically be conforming anymore.

Since [[nodiscard]] has no normative effect, both of our examples are technically conforming, with and without this patch.

Whenever we do an API-breaking change, it's useful to give customers a way to opt out of it.

If a user doesn't want to/can't fix their broken code, there are still two ways they can opt out:

  • Cast-to-void
  • Enable -Wno-unused-result

This could piss off customers with big old codebases.

I don't think we should hedge on the possibility of someone getting upset with libc++ because we now flag their buggy/questionable code (see my commit message). Let's apply it as-is, and if there are a substantial number of complaints, we can consider adding a libc++-specific knob then.

Quuxplusone added inline comments.Aug 15 2021, 4:09 PM
libcxx/docs/UsingLibcxx.rst
274–281

"Cast-to-void" counts as "fixing their broken code" (since that is a code change). Customers who "can't" change their code (e.g. because it's third-party code) generally can't even go patching it to add casts-to-void; they have to find solutions that operate strictly within the build system. Passing -Wno-unused-result is a legit solution. I just worry that it might be too much of a blunt instrument — it means there's no way for customers to opt out of the new aggressive warnings without also opting out of the old warnings (which by definition they'd already considered harmless).

Also, note that by "adding a knob," I really mean just "not removing the knob we already have." This PR consists basically of two parts: (1) changing __config, and (2) sed -i 's/_EXT//' include/*. I'm basically suggesting that we do (1) but not (2). You're suggesting that we do (1) and (2) now, and then later consider reverting only the (2) part. At least let's commit this in two pieces — the part-(1) piece I bet we'll never even consider reverting, and the part-(2) piece that we might end up reverting. That way we don't have to do all this work twice.

Data point: I ran this patched libc++ against my employer's codebase (which includes a large swath of Boost); it did not detect any true or false positives. Which is a good sign: out of a sample size of 1, I found 0 customers complaining about the effect of this PR. :)

I'm also not happy with the current layers of nodiscard. I only wonder whether this is the right approach. I'm concerned whether the change will result in a lot of complaints after we release this change.

libcxx/docs/UsingLibcxx.rst
274–281

With this PR, libc++ will refuse to compile that program, so libc++ won't technically be conforming anymore.

Since [[nodiscard]] has no normative effect, both of our examples are technically conforming, with and without this patch.

While this it correct in theory, I don't think it's correct in practice.

Whenever we do an API-breaking change, it's useful to give customers a way to opt out of it.

If a user doesn't want to/can't fix their broken code, there are still two ways they can opt out:

  • Cast-to-void
  • Enable -Wno-unused-result

This could piss off customers with big old codebases.

I don't think we should hedge on the possibility of someone getting upset with libc++ because we now flag their buggy/questionable code (see my commit message). Let's apply it as-is, and if there are a substantial number of complaints, we can consider adding a libc++-specific knob then.

I don't necessarily disagree. I wonder how we can test the number of complaints. I expect most users will discover the change when they start using Clang 14, which is too late to fix libc++. Some users will never use Clang 14, but encounter this with Clang 1x shipped with Ubuntu 2024 LTS.

I had a talk with the MSVC STL folks at discord since they have more experience in using [[nodiscard]] liberally. Their experience takes away my concerns. I think it would still be good to see whether this patch can be tested on a larger code base, chromium for example to see whether it has false positives.

cjdb added a comment.Aug 16 2021, 12:21 PM

I had a talk with the MSVC STL folks at discord since they have more experience in using [[nodiscard]] liberally. Their experience takes away my concerns. I think it would still be good to see whether this patch can be tested on a larger code base, chromium for example to see whether it has false positives.

You read my mind. Since I'm in no hurry to merge this patch, how about I test it on various Google codebases and see what breaks? That's at least half a billion lines of code (probably more) that we can get data from. Doing that takes a bit of time, so don't expect an answer before the end of the week. Since I'm heavily biased against anything other than _LIBCPP_NODISCARD, I don't think I should be the one to set what we should consider statistically significant, but I'm going to offer 0.001% of the codebase as a starting threshold (if my estimate is accurate then that's 5k offences). If you think that percentage is too high or too low, let me know and we can discuss (preferably on Discord).

Since I'm in no hurry to merge this patch, how about I test it on various Google codebases and see what breaks? That's at least half a billion lines of code (probably more) that we can get data from. Doing that takes a bit of time, so don't expect an answer before the end of the week. Since I'm heavily biased against anything other than _LIBCPP_NODISCARD, I don't think I should be the one to set what we should consider statistically significant, but I'm going to offer 0.001% of the codebase as a starting threshold

If you find zero false positives (that is, if you find nothing; or if you find only things that I agree are bugs) then personally I'll accept the sample size of "2 codebases, 0 harm" as good enough for the "just ship it and wait for customer complaints" strategy. However, if you find something — that is, if applying this patch does break Google's build — then personally I'd consider that evidence in favor of the more cautious strategy. I don't think there's any need to do math here.

ldionne requested changes to this revision.Aug 30 2021, 1:09 PM

IMO, this approach makes sense. However, from 99% of the people's perspective (who didn't enable the extensions), this is equivalent to suddenly adding [[nodiscard]] on a bunch of algorithms, period. This has widespread potential for breakage (I'm not saying it does, but it has the potential to).

@cjdb Can you share the results of running this on Google's code base? I'll do the same at Apple and report when that's done. If we don't have significant deployment issues to report, I'd be fine with doing this.

Requesting changes for now.

This revision now requires changes to proceed.Aug 30 2021, 1:09 PM

@cjdb Can you share the results of running this on Google's code base? I'll do the same at Apple and report when that's done. If we don't have significant deployment issues to report, I'd be fine with doing this.

Note to self: 82544962

cjdb added a comment.Aug 30 2021, 2:11 PM

IMO, this approach makes sense. However, from 99% of the people's perspective (who didn't enable the extensions), this is equivalent to suddenly adding [[nodiscard]] on a bunch of algorithms, period. This has widespread potential for breakage (I'm not saying it does, but it has the potential to).

@cjdb Can you share the results of running this on Google's code base? I'll do the same at Apple and report when that's done. If we don't have significant deployment issues to report, I'd be fine with doing this.

Requesting changes for now.

I will, but it's going to take me some time to get accurate results. I'm also compiling it into a table per codebase, with the following information:

FunctionTotal estimated usageBuild failures due to [[nodiscard]]Owners agreed failure was a bug
fn~K hitsNot mentioned == zero hitsx/failures owners agreed

Gathering such data is a lot slower but gives a lot more insight (e.g. ::operator new has zero hits in two of the codebases, but that'd be obfuscated if I ran it alongside std::remove). I also need to run this on codebases where I can't directly apply this patch too, which will take more time than I'd first thought.

Okay, so FWIW, I ran this on a significant code base internally and found only a reasonable number of hits, all of which appeared to be bugs just waiting to happen. So I'm supportive of this as a general idea.

Here's how I'd like us to do this:

  1. I'd like us to keep _LIBCPP_NODISCARD_EXT as a documentation of what is a libc++ specific extension and what is not.
  2. We should make _LIBCPP_NODISCARD_EXT enabled by default, and add a knob that allows removing it. We can document the knob that allows removing those extensions *and mention that the knob will be removed within 2 releases*.
  3. In two releases, we remove the knob and _LIBCPP_NODISCARD_EXT is always enabled, period.

That will let code bases adopt the change incrementally over roughly one year. In particular, I'd like to avoid significant breakage in open-source code bases without giving them a way to opt-out until they've had the time to fix their errors. WDYT?

Thanks for running this test!
It would still be nice to see whether @cjdb's results match with your results and the feedback of Microsoft. (Unless that's a lot of effort.)

I think this is a good way forward. Let's make sure we update the release notes in this patch.
I think it would be nice to issue a diagnostic when the user turns on the knob, that way they know it's only a temporary solution; the real solution is to fix their code.

A transition period of a year sounds reasonable.

@ldionne wrote:

Here's how I'd like us to do this:
I'd like us to keep _LIBCPP_NODISCARD_EXT as a documentation of what is a libc++ specific extension and what is not.
We should make _LIBCPP_NODISCARD_EXT enabled by default, and add a knob that allows removing it. We can document the knob that allows removing those extensions and mention that the knob will be removed within 2 releases.
In two releases, we remove the knob and _LIBCPP_NODISCARD_EXT is always enabled, period.

I'm happy with the first 1.5 parts, and skeptical-of-but-not-objecting-to the second 1.5 parts (the part where you can ever actually remove the deprecated knob). To make sure we're on the same page, though: You're proposing that two releases from now we remove the _LIBCPP_DISABLE_NODISCARD_EXT macro and its tests, but we continue marking all our extensions using the _LIBCPP_NODISCARD_EXT macro, right? You're not proposing that two releases from now we do a massive search-and-replace of _LIBCPP_NODISCARD_EXT into [[nodiscard]], right? (Because I would object to that; and I also think it'd have logistical problems because C++03 doesn't support [[nodiscard]] — all the same arguments from _EnableIf apply here. Fortunately, my understanding is that you're not proposing such a massive search-and-replace, so it's a moot point, thank you.)
TLDR I'd like to be reassured that we're not search-and-replacing _LIBCPP_NODISCARD_EXT itself; but assuming I'm right about that, then your plan sounds perfect.