Page MenuHomePhabricator

[libcxx] Slightly improved policy for handling experimental features
ClosedPublic

Authored by ldionne on May 24 2019, 2:26 PM.

Details

Summary

Following the discussion on the libcxx-dev mailing list
(http://lists.llvm.org/pipermail/libcxx-dev/2019-May/000358.html),
this implements the new policy for handling experimental features and
their deprecation. We basically add a deprecation warning for
std::experimental::filesystem, and we remove a bunch of <experimental/*>
headers that were now empty.

Diff Detail

Repository
rL LLVM

Event Timeline

ldionne created this revision.May 24 2019, 2:26 PM

Basically, we're comfortable with giving this a try and seeing whether we still get into trouble. If so, we might push again for a -fexperimental flag, however for now the simple approach taken here might be sufficient.

Also note that we might want a -fexperimental flag just to control whether -lc++experimental is being linked, however this is left as a separate change if we decide we want to do it.

daphnediane added a subscriber: daphnediane.EditedMay 27 2019, 1:18 PM

Not all package systems are good about cleaning up removed headers so the approach given by this patch could result in deprecated headers being left behind after an update instead of removed.

I wonder if something should be done with __has_include also as it seems like this could be a more generic problem for deprecated headers for more than just the TS experimental headers. Maybe have __has_include immediately check after finding a matching file if a non-empty file with the suffix replaced with .rmh exists in the same path evaluate to zero. Likewise if #include ( but not #include_next ) falls to find a file it could search for a the file with the suffix replaced with .rmh and if found print an error message about a deprecated header possibly including some content from the .rmh header with details. This would allow a header to be deprecated but still give useful messages if used while not breaking __has_include. It would also help with cases where an updated library was installed where headers were removed and the old headers did not get rewritten with an installed package.

Edit: Tweaked wording, avoiding __ in __has_include acting as markup.

Not all package systems are good about cleaning up removed headers so the approach given by this patch could result in deprecated headers being left behind after an update instead of removed.

Thanks a lot for raising this point.

However, I'll admit that I'm personally uncomfortable with working around broken package managers with that sort of hack, unless we have an extremely compelling reason to do it (e.g. an important package manager doesn't do it, and can't be fixed because of <insert reasons>).

The deletions are fine, the doc updates are good, but I think deprecation warnings are not.
Things that are going to happen in the future (a year from now, say) or never (if people don't update their tools) don't belong in the warning spew for every single build

jfb added a comment.May 28 2019, 11:16 AM

The deletions are fine, the doc updates are good, but I think deprecation warnings are not.
Things that are going to happen in the future (a year from now, say) or never (if people don't update their tools) don't belong in the warning spew for every single build

Similar deprecation warnings are pretty common coming from clang (both for standard things as well as extensions / builtins). What Louis proposes seems consistent with what the compiler does. Do you think there's a better approach that libc++ could take?

jfb added inline comments.May 28 2019, 11:20 AM
libcxx/docs/DesignDocs/ExperimentalFeatures.rst
25 ↗(On Diff #201336)

I generally capitalize "Standard".

39 ↗(On Diff #201336)

I think you want to specify: when implemented fully. Sometimes we can ship something that's not complete yet, and we can't start the deprecation clock until the implementation is complete.

46 ↗(On Diff #201336)

2 releases of LLVM? What do we call "open source public Official LLVM"?

139 ↗(On Diff #201336)

toto africa?

libcxx/docs/UsingLibcxx.rst
89 ↗(On Diff #201336)

Link to the policy from here?

The deletions are fine, the doc updates are good, but I think deprecation warnings are not.
Things that are going to happen in the future (a year from now, say) or never (if people don't update their tools) don't belong in the warning spew for every single build

Okay, so we've had offline discussions about this, and the general idea I got is that you would prefer that these warnings be in a tool like clang-tidy, that users can run only when they so desire. Please correct me if that's incorrect.

However, the key thing here is that I would like for these warnings to be opt-OUT, not opt-IN. This is really important for me, as otherwise most users are not even aware of the situation and they don't turn the warnings on (aka run the tool). Hence, using clang-tidy or any similar solution that is NOT in the mandatory compilation workflow is not sufficient.

Furthermore, I think this is, at the end of the day, a vendor decision. I don't think having these warnings (or not having them) makes much of a difference for libc++ as an open source project, however it makes a big difference for vendors. I feel like this issue is important enough that we would do this downstream if we were only given that choice. However, one thing I would suggest in order to resolve the stalemate is to guard these deprecation warnings behind a CMake configuration option (for example). That way, vendors can decide whether they want their libc++ to contain the warnings or not, and upstream libc++ can decide to do differently. The downside with this solution is that we add yet another configuration that must be tested, but it's better than nothing.

@mclow.lists How do you feel about that compromise?

In D62428#1519664, @jfb wrote:

The deletions are fine, the doc updates are good, but I think deprecation warnings are not.
Things that are going to happen in the future (a year from now, say) or never (if people don't update their tools) don't belong in the warning spew for every single build

Similar deprecation warnings are pretty common coming from clang (both for standard things as well as extensions / builtins). What Louis proposes seems consistent with what the compiler does. Do you think there's a better approach that libc++ could take?

I think they're wrong there, too.
I've enumerated why I believe so on the ML, and in Slack (several times), so I don't see the point of doing it again here.
[ Though I will note that no one has argued that my reasons are incorrect or invalid. ]

In D62428#1519664, @jfb wrote:

The deletions are fine, the doc updates are good, but I think deprecation warnings are not.
Things that are going to happen in the future (a year from now, say) or never (if people don't update their tools) don't belong in the warning spew for every single build

I don't understand your point here Marshall. Why would this cause spew for every build? Users can choose to change their code, or to opt-out of the diagnostic.

If this opt-out mechanism isn't quite right, we can invent new ones. For example, we could use __attribute__((deprecated)) or extend __attribute__((availability)) somehow so that the diagnostics are bucketed under -Wdeprecated. We could create a special -Wdeprecated-experimental diagnostic so users can opt-out of (or -in to, via -Wno-deprecated -Wdeprecated-experimental) just those.

Similar deprecation warnings are pretty common coming from clang (both for standard things as well as extensions / builtins). What Louis proposes seems consistent with what the compiler does. Do you think there's a better approach that libc++ could take?

I think they're wrong there, too.
I've enumerated why I believe so on the ML, and in Slack (several times), so I don't see the point of doing it again here.
[ Though I will note that no one has argued that my reasons are incorrect or invalid. ]

I think everyone respects your opinion, and there isn't only one right answer here. I can understand why some vendors would prefer to relegate deprecation notices entirely to release notes, or to make them opt-in. I can also understand why some users would choose to opt-out of all deprecation notices (e.g., via -Wno-deprecated).

As a vendor, we want our users to know immediately about deprecations so that they have maximum time to plan how to update their code. We want users to be able to suppress deprecation warnings (a) outright, in case they don't want this kind of notice, or (b) temporarily, to avoid noise their builds while they make plans for how to update them.

It's clear that you personally disagree with this policy, and I want to acknowledge that that's a valid opinion. Nevertheless, given that this policy is fully supported by Clang and we've followed it in Libc++ in the past (e.g., when we added diagnostics to the libstdc++ compatibility headers), it's surprising that you feel so strongly that you might block this entirely, even as a vendor configuration option.

I'm curious, as a thought experiment, let's say you agreed to support vendors that wanted to continue with this policy. Do you have thoughts on this way of configuring whether vendors adopt it? Do have you have ideas for different or better opt-out mechanisms (through some combination of -W or -D flags, or otherwise)?

libcxx/include/experimental/filesystem
241 ↗(On Diff #201336)

IMO, we should find a way to allow users to opt-out via -Wno-deprecated, in addition to (or instead of) macros.

Folks, I just spoke to Marshall over the phone and I believe one of the reasons for the opposition is that I didn't make it clear we were not warning on ALL uses of experimental features. The new proposal only adds deprecation warnings when we standardize a feature and we know FOR SURE we're going to remove the experimental version in the near future. I didn't make that change from the initial proposal clear, and I apologize for that. With that thing being clear, the proposal seemed to be more reasonable.

Now, it's not to say that this clears out ALL opposition. We agreed to hold a wider-audience discussion about the place of warnings (and deprecation warnings in particular) inside the compiler versus a separate tool (e.g. clang-tidy). I'm very much looking forward to that discussion because right now we only have a hammer (the compiler), and as a result everything looks like a nail. However, we decided not to make that discussion a prerequisite for the policy that is the object of this patch.

Marshall, I'm looking forward to your comments on the proposal, and to the discussion on cfe-dev about deprecation warnings in the compiler in general!

mclow.lists accepted this revision.May 31 2019, 12:23 PM

Folks, I just spoke to Marshall over the phone and I believe one of the reasons for the opposition is that I didn't make it clear we were not warning on ALL uses of experimental features. The new proposal only adds deprecation warnings when we standardize a feature and we know FOR SURE we're going to remove the experimental version in the near future. I didn't make that change from the initial proposal clear, and I apologize for that. With that thing being clear, the proposal seemed to be more reasonable.

I still think that this is the wrong place for this functionality, but this solves a problem today, and we can fix it "better" later.
P.S. I like @dexonsmith 's suggestion to hook into -Wno-deprecated somehow.

This revision is now accepted and ready to land.May 31 2019, 12:23 PM
ldionne updated this revision to Diff 202469.May 31 2019, 12:36 PM
ldionne marked 7 inline comments as done.

Address reviewer's comments

ldionne added inline comments.May 31 2019, 12:38 PM
libcxx/docs/DesignDocs/ExperimentalFeatures.rst
46 ↗(On Diff #201336)

I think saying "2 releases of LLVM" is unambiguous, as vendors are downstream (and hence in some sense invisible to the eyes of this document).

libcxx/include/experimental/filesystem
241 ↗(On Diff #201336)

We definitely want to allow users to disable warnings for each feature, because otherwise when they disable the warnings for one feature, they stop getting warnings for all other experimental features, which defeats the purpose of this proposal.

However, using the [[deprecated]] attribute does make sense since it _allows_ users to turn the warnings off globally if they want to do so, but they can also turn off just the warnings they want.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 7:48 AM