llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.63k stars 11.83k forks source link

Can the diagnostic for use of a qualified free function type be improved? #64402

Open philnik777 opened 1 year ago

philnik777 commented 1 year ago

Currently, clang accepts typedef void Func() const; for some reason, while rejecting this construct in most other cases (e.g. when trying to make a pointer to it). It gets really weird when using it with classes. e.g.

using Func = void() const;

template <class T>
class S {
  T* t;
};

int main() {
  S<Func> f; // expected-error {{pointer to function type 'void () const' cannot have 'const' qualifier}}
}

Clang will happily accept this code when not using T inside S.

llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-frontend

cor3ntin commented 1 year ago

This is abominable (Abominable Function Type is the unofficial name WG21 give to these things), but valid - Lots of information in the paper. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0172r0.html

philnik777 commented 1 year ago

Ok, that is really weird. Could we at least get a better error in such cases? GCCs error message is way better here (IMO): https://godbolt.org/z/a1oar5MWd

dwblaikie commented 1 year ago

I dunno, I actually think the Clang one is a bit better - it includes the point of instantiation and says what's wrong, whereas the GCC just says "doing a thing" and doesn't say which part of that thing (if any) is the problem.

philnik777 commented 1 year ago

I dunno, I actually think the Clang one is a bit better - it includes the point of instantiation and says what's wrong, whereas the GCC just says "doing a thing" and doesn't say which part of that thing (if any) is the problem.

It reads to me like clang is saying that I cannot qualify T* with const, which doesn't make any sense, since there is no const anywhere near it. GCC says that I cannot form a pointer to void() const. Maybe something like "Cannot form pointer to qualified function prototype" would be better? IDK.

AaronBallman commented 1 year ago

I think the current diagnostic behavior in Clang is reasonable (I think it's better than GCC's diagnostic). The note clarifies where the const is coming from, and I think the diagnostic stack is actionable. That said, I'd also be fine changing the diagnostic to "cannot form a pointer to a qualified function type (%0)" (and print the qualified function type) or something along those lines.

philnik777 commented 1 year ago

I think the current diagnostic behavior in Clang is reasonable (I think it's better than GCC's diagnostic). The note clarifies where the const is coming from, and I think the diagnostic stack is actionable. That said, I'd also be fine changing the diagnostic to "cannot form a pointer to a qualified function type (%0)" (and print the qualified function type) or something along those lines.

What note says where the const is coming from? The only note I see says that it's been instantiated somewhere. If it pointed to the const it would probably have been much less confusing to me.

AaronBallman commented 1 year ago

<source>:9:11: note: in instantiation of template class 'S<void () const>' requested here

void () const is explicitly spelled out as the template type argument (the code snippet shows the code as-written, with S<Func>), which corresponds to T in the class context.

So we don't point directly to the const, we point to the const-qualified type.

philnik777 commented 1 year ago

Ok, I'm completely confused what this construct actually is. Is this literally a const qualified void()? Because using Func = void(); S<const Func>{} doesn't work. Or is this special-cased somehow? Anyways, I think the error message might make sense when you know what's going on, but I don't and am completely confused. I think saying that you're trying to form a pointer to a qualified functions type makes the situation a lot clearer, but maybe I'm alone in this.

AaronBallman commented 1 year ago

Ok, I'm completely confused what this construct actually is.

You're not alone -- this construct is deeply weird.

Is this literally a const qualified void()?

Yup! That's what makes it so deeply weird.

Because using Func = void(); S<const Func>{} doesn't work. Or is this special-cased somehow?

It's a special case because of how the language rules fall out. https://wg21.link/p0172 has gory details, but see section 2.5 for why these things exist at all.

Anyways, I think the error message might make sense when you know what's going on, but I don't and am completely confused. I think saying that you're trying to form a pointer to a qualified functions type makes the situation a lot clearer, but maybe I'm alone in this.

I have no problems if someone wants to try to improve the diagnostic behavior, but given how hard it is to run into the problem in the first place, it may not be worth the effort (IIRC, this diagnostic falls out from more generic type system code; it could be easy to change the diagnostic, it could be a challenge, I've not investigated that hard enough to know).

AaronBallman commented 1 year ago

(I took a stab at changing the issue title; feel free to adjust how you'd like!)

philnik777 commented 1 year ago

Ok, I'm completely confused what this construct actually is.

You're not alone -- this construct is deeply weird.

Is this literally a const qualified void()?

Yup! That's what makes it so deeply weird.

Because using Func = void(); S<const Func>{} doesn't work. Or is this special-cased somehow?

It's a special case because of how the language rules fall out. https://wg21.link/p0172 has gory details, but see section 2.5 for why these things exist at all.

Anyways, I think the error message might make sense when you know what's going on, but I don't and am completely confused. I think saying that you're trying to form a pointer to a qualified functions type makes the situation a lot clearer, but maybe I'm alone in this.

I have no problems if someone wants to try to improve the diagnostic behavior, but given how hard it is to run into the problem in the first place, it may not be worth the effort (IIRC, this diagnostic falls out from more generic type system code; it could be easy to change the diagnostic, it could be a challenge, I've not investigated that hard enough to know).

Thanks for being so patient with me! I agree that it's quite hard to run into this, given that I worked on libc++ for two years and only noticed this in a test for specifically this case - and thought the test was wrong. Ironically I even used this while implementing move_only_function, but didn't make the connection until now. I might take a stab at improving the diagnostic myself at some point, but I don't think I'm familiar enough with clang internals yet to try that.

(I took a stab at changing the issue title; feel free to adjust how you'd like!)

Sounds fine to me.