This is an archive of the discontinued LLVM Phabricator instance.

[libc++] type_traits: use __is_core_convertible in __invokable_r.
ClosedPublic

Authored by jacobsa on May 10 2022, 3:38 AM.

Details

Summary

This fixes incorrect handling of non-moveable types, adding tests for this case.
See issue 55346.

The current implementation is based on is_convertible, which is
defined in terms of
validity of the following function:

To test() {
  return declval<From>();
}

But this doesn't work if To and From are both some non-moveable type, which the
definition of implicit
conversions says should work due to guaranteed copy elision:

To to = E;  // E has type From

It is this latter definition that is used in the
definition
of INVOKE<R>. Make invokable_r use is_core_convertible, which
captures the ability to use guaranteed copy elision, making the
definition correct for non-moveable types.

Fixes llvm/llvm-project#55346.

Diff Detail

Event Timeline

jacobsa created this revision.May 10 2022, 3:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 3:38 AM
jacobsa requested review of this revision.May 10 2022, 3:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 3:38 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jacobsa updated this revision to Diff 428330.May 10 2022, 3:42 AM
jacobsa edited the summary of this revision. (Show Details)

Remove the "fixes" part of the description.

I forgot that I need another commit for std::is_nothrow_invocable_r
afterward.

philnik requested changes to this revision.May 10 2022, 4:26 AM
philnik added a subscriber: philnik.

You can also put is_nothrow_invocable_r in this PR if you want. The PR is small enough.

libcxx/include/type_traits
3172–3193

We normally don't put comments to explain the standard. We have standard for documentation.

3182–3185

Is there a reason you don't fix __invokable_r?

3194

This seems more in line with how invoke_result is used.

3195

Why are you inheriting from is_convertible and then overwrite all the members?

3199

We usually use class and not typename.

3200

You have to _Uglify all the names. These functions shouldn't be static. They should never be instantiated anyways. Could you rename this to _IsImplicitlyConvertible or something similar? That would remove the need for the comment above since the code is self explanatory.

3202–3204

Couldn't you just use std::declval?

3211

We use these meta programming techniques all over the place. We don't have to document what this does.

3228
libcxx/test/libcxx/type_traits/is_invocable_r.pass.cpp
1 ↗(On Diff #428330)

This file is missing the license header. It should also be in test/std since it's standard behaviour and nothing libc++ specific.

5–8 ↗(On Diff #428330)

Same for the other tests

philnik added inline comments.May 10 2022, 4:26 AM
libcxx/test/libcxx/type_traits/is_invocable_r.pass.cpp
2 ↗(On Diff #428330)

You should also put the synopsis here.

93–97 ↗(On Diff #428330)

You can make this file a .compile.pass.cpp. Then you don't have to add a main.

This revision now requires changes to proceed.May 10 2022, 4:26 AM
jacobsa updated this revision to Diff 428550.May 10 2022, 7:08 PM
jacobsa marked 7 inline comments as done.

Address comments from philnik.

@philnik: thanks for the quick review! Sorry if I've made any dumb mistakes
here; this is my first time contributing and I'm not familiar with the style,
processes, or tools.

I've left is_nothrow_invocable_r out of this PR to keep it as small as
possible. After learning from this one, the next will require fewer comments.
:-)

By the way, do you know what's up with the failed AIX build? It seems like this
is a pre-existing failure; is it common for that to happen?

libcxx/include/type_traits
3194

Sorry, I'm not sure what you mean here; could you elaborate? "Convert from
_From to _To" seems like a natural order, and matches is_convertible.

3195

Oops, this was an editing mistake. Removed; thanks for catching.

3200

I've uglified the names. Can you tell me why it's necessary to do so? I've
never understood this, since we're already in the scope of an internal struct.

I'm not sure these can/should be non-static, since it messes with the ability
to have a constant expression for the decltype tests below, and requires them
to be a terrible mouthful. See below, which also doesn't work for a reason I
haven't dug into. static seems preferable to this. Do you think this is
important?

template <class _Tp, class = decltype(std::declval<__is_implicitly_convertible>()
                                          .template _RequireImplicitlyConvertibleTo<_To>(_Make<_Tp>()))>
true_type _TestImplicitConversion(int);

[...]

using type = decltype(std::declval<__is_implicitly_convertible>().template _TestImplicitConversion<_From>(0));
3202–3204

That results in an rvalue reference, so it doesn't work correctly for
non-moveable types. The point is to lean on guaranteed copy elision to give the
right answer for those types, matching the standard which defines this in terms
of initialization from an expression of type _From, not _From&&.

libcxx/test/libcxx/type_traits/is_invocable_r.pass.cpp
2 ↗(On Diff #428330)

Done, although I'm not sure if there is a standard form of synopsis. I've put
something very brief here, because it seems like it's self-explanatory from the
name of the file.

93–97 ↗(On Diff #428330)

Ah, nice tip. Done.

philnik added inline comments.May 11 2022, 1:24 AM
libcxx/include/type_traits
3194

I think it's debatable whether _From _To is the natural order. I always have to look that up. But I'm Ok with keeping it the same as is_convertible.

3200

I've uglified the names. Can you tell me why it's necessary to do so? I've never understood this, since we're already in the scope of an internal struct.

We have to _Uglify all the names because a User might #define Make "This is definitely not an identifier" and it is perfectly legal. We don't have to do the same shenanigans in files that the user will never see the internals of. You might notice that we don't _Uglify all the names in libcxx/src. There we only _Uglify the public ones.
So at some point in the future, when modules are used everywhere and the standard doesn't define things in terms of headers anymore we can stop _Uglifying the new code. So we can probably stop in 10 years if we're lucky.

I'm not sure these can/should be non-static, since it messes with the ability to have a constant expression for the decltype tests below [...]

Oops, I forgot that they are part of a struct. Ignore that part.

libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp
12
75

Could you also add tests with arguments?

80
84
92

Could you also add all the tests for is_invocable_r?

Sorry I missed this: The AIX 32 bit build is currently broken in trunk. You can ignore that failure.

jacobsa updated this revision to Diff 428600.May 11 2022, 2:50 AM
jacobsa marked 3 inline comments as done.

Update based on comments from philnik.

Dumb question: is there a way to see a list of unresolved comments? All I can
see how to do is look at _all_ the comments in the diff view, which is hard
to parse when there are a lot of resolved ones. I'm not 100% sure I addressed
all of your comments.

libcxx/include/type_traits
3200

Ah right, thanks. I forgot about the preprocessor!

libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp
84

It's this way on purpose: this is showing that it's possible to use the
function to initialize a CantMove, i.e. no move constructor is necessary for
such a result type when invoking the function.

92

I assume you don't mean _all_ the tests, right? That seems unnecessary; we're
a lot more likely to have bugs and inconsistencies in the two sets of tests
than for the two implementations to be out of sync. I added a test confirming
that it exists and works. PTAL.

Dumb question: is there a way to see a list of unresolved comments? All I can
see how to do is look at _all_ the comments in the diff view, which is hard
to parse when there are a lot of resolved ones. I'm not 100% sure I addressed
all of your comments.

I don't know what you are looking at exactly, but on the top right you should see something like 12 / 23 Comments. You can click on that to go to the next unresolved comment. Right next to that you can also select Hide "done" Inlines.

libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp
84

Right. My brain probably went "Oh look, a value initialization with the type spelled out twice; let's just spell it once".

92

No, I meant all the tests. The tests should be able to test all standard library implementations and not just libc++. This means that we shouldn't just test code paths we know exist, but also ones which could very well exist. is_invocable_r and is_invocable_r_v could be implemented completely differently, so we should test for everything. This also future-proofs the tests. We almost never change any of the tests once they are written because the API is set in stone from now 'till the end of time (or until at some point the standard breaks the API, which hasn't happened in the last 25 years). If anything we almost always only add new tests.

jacobsa added a comment.EditedMay 11 2022, 6:25 PM

Dumb question: is there a way to see a list of unresolved comments? All I can
see how to do is look at _all_ the comments in the diff view, which is hard
to parse when there are a lot of resolved ones. I'm not 100% sure I addressed
all of your comments.

I don't know what you are looking at exactly, but on the top right you should see something like 12 / 23 Comments. You can click on that to go to the next unresolved comment. Right next to that you can also select Hide "done" Inlines.

I see that this is supposed to work; there is even a screenshot here:

https://secure.phabricator.com/rPb3b30dde6ac99786a77f903823ebf259db6e3adc

But my diff view doesn't look like that; it doesn't have any of the widgets
shown in that screenshot. Here's what I see: https://imgur.com/FdJwQz1

libcxx/include/type_traits
3182–3185

Sorry, I don't know how I missed this. Done, and i see that this now fixes
is_nothrow_invokable_r too, so I've added a test for that. In that case I
haven't duplicated the test for _v, since the existing test file doesn't do
that with all the other cases.

libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp
92

Ack, done.

jacobsa updated this revision to Diff 428828.May 11 2022, 6:28 PM
jacobsa retitled this revision from type_traits: rewrite is_invocable_r in terms of argument passing. to type_traits: make __invokable_r work in terms of argument passing..
jacobsa edited the summary of this revision. (Show Details)

Update based on feedback from philnik. Fix __invokable_r instead.

FYI, I messed up and submitted the above reply early, sorry. Please take
another look if you're reading by email; I've edited the comment.

jacobsa updated this revision to Diff 428830.May 11 2022, 6:30 PM
jacobsa edited the summary of this revision. (Show Details)

Put "fixes" back into the commit message now that this also handles
is_nothrow_invocable_r.

philnik accepted this revision as: philnik.May 12 2022, 1:12 AM

Dumb question: is there a way to see a list of unresolved comments? All I can
see how to do is look at _all_ the comments in the diff view, which is hard
to parse when there are a lot of resolved ones. I'm not 100% sure I addressed
all of your comments.

I don't know what you are looking at exactly, but on the top right you should see something like 12 / 23 Comments. You can click on that to go to the next unresolved comment. Right next to that you can also select Hide "done" Inlines.

I see that this is supposed to work; there is even a screenshot here:

https://secure.phabricator.com/rPb3b30dde6ac99786a77f903823ebf259db6e3adc

But my diff view doesn't look like that; it doesn't have any of the widgets
shown in that screenshot. Here's what I see: https://imgur.com/FdJwQz1

You have to scroll down a bit further. Then a banner should pop up at the top of the screen where the other options are. BTW if you want some of your questions get answered a bit quicker you might want to join the LLVM discord.

LGTM % nits. If you don't have commit access (which I assume) please provide "Your Name" <your.email@address.com> for attribution.

libcxx/include/type_traits
2998–2999

Could you add a comment why you can't just use std::declval? I think that's not obvious.

libcxx/test/std/utilities/meta/meta.rel/is_nothrow_invocable.pass.cpp
200–201

Could you also add static_asserts for the _v version?

philnik retitled this revision from type_traits: make __invokable_r work in terms of argument passing. to [libc++] type_traits: make __invokable_r work in terms of argument passing..May 12 2022, 1:13 AM
jacobsa updated this revision to Diff 429137.May 12 2022, 9:43 PM
jacobsa marked 12 inline comments as done.

Address latest comments.

Thanks! You can credit Aaron Jacobs <jacobsa@google.com>.

@philnik sorry if this is an obvious question, but what's the next step here?
Are you able to approve the change so I can commit it, or do you commit
giving me attribution, or what?

@philnik sorry if this is an obvious question, but what's the next step here?
Are you able to approve the change so I can commit it, or do you commit
giving me attribution, or what?

A second person from the #libc group has to approve and then I (or someone else) can commit it. The review pipeline is currently stalled a bit so it might take another week, probably mostly depending on when @ldionne has some time. Feel free to ping in a week or so if nothing happened until then.

jloser added a subscriber: jloser.May 14 2022, 9:20 PM
jloser added inline comments.
libcxx/include/type_traits
2986

// Detects whether an expression of type _From can be implicitly converted to _To

2989

// function and therefore gives the wrong answer for non-movable types.

jacobsa updated this revision to Diff 429518.May 15 2022, 12:09 AM
jacobsa marked 2 inline comments as done.

Fix typos pointed out by jloser.

EricWF added a subscriber: EricWF.May 20 2022, 11:31 AM

I think we already have a trait for this. It's __is_core_convertible. Could you use that instead?

libcxx/include/type_traits
2993

Use __is_core_convertible, which already addresses this.

EricWF requested changes to this revision.May 20 2022, 12:09 PM

Requesting the __core_is_convertible changes formally to put this review in the correct bucket.

libcxx/include/type_traits
3002

Not sure this comment adds anything.

This revision now requires changes to proceed.May 20 2022, 12:09 PM
jacobsa updated this revision to Diff 431102.May 20 2022, 5:13 PM
jacobsa retitled this revision from [libc++] type_traits: make __invokable_r work in terms of argument passing. to type_traits: use __is_core_convertible in __invokable_r..
jacobsa edited the summary of this revision. (Show Details)

Use __is_core_convertible rather than reimplementing it.

jacobsa marked 2 inline comments as done.May 20 2022, 5:14 PM

Thanks Eric, PTAL.

libcxx/include/type_traits
2993

I didn't know this existed. That's much better, thanks.

philnik added inline comments.May 20 2022, 5:17 PM
libcxx/include/type_traits
2997–2998

Well, that's a lot shorter. Although I have to say __is_core_convertible isn't a very intuitive name. Is there any reason it is called that @EricWF?

@jacobsa could you add [libc++] to the name of the PR?

jacobsa updated this revision to Diff 431106.May 20 2022, 5:47 PM
jacobsa marked an inline comment as done.
jacobsa retitled this revision from type_traits: use __is_core_convertible in __invokable_r. to [libc++] type_traits: use __is_core_convertible in __invokable_r..

Add [libc++] to the title.

EricWF accepted this revision.May 23 2022, 5:58 PM

Thanks for the fix!

libcxx/include/type_traits
2997–2998

I think it refers to the "core language" definition of convertibility rather than the library definition?

This revision is now accepted and ready to land.May 23 2022, 5:58 PM

@EricWF @philnik Thanks! But how do I actually commit this? Do you need to do it for me?

This revision was landed with ongoing or failed builds.May 24 2022, 1:24 AM
This revision was automatically updated to reflect the committed changes.