[Clang] `++this` with a dependent `this` is accepted when it’s not instantiated

[Sirraide]

This code compiles, unless we instantiate and call f(). This doesn’t seem right to me because, irrespective of what the type of e.g. this ends up being, it’s a prvalue, and we can’t increment that.

template <typename T>
struct S : T {
    auto t() -> S*;
    void f() { 
        ++reinterpret_cast<S*>(4);
        ++this;
        ++t(); 
    }
};

Imo we should diagnose this earlier if that is feasible. GCC also suffers from the same problem, which caused ++this as a typo for ++*this to go unnoticed in a libstdc++ header because the template containing it was never instantiated by libstdc++. MSVC also accepts this code. Is this a bug or a standards defect? Because I don’t see a situation where ++this would be semantically valid.

CC @AaronBallman, @cor3ntin, @erichkeane

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: None (Sirraide)

This code compiles, unless we instantiate and call `f()`. This doesn’t seem right to me because, irrespective of what the type of e.g. `this` ends up being, it’s a prvalue, and we can’t increment that.

template <typename T>
struct S : T {
    auto t() -> S*;
    void f() { 
        ++reinterpret_cast<S*>(4);
        ++this;
        ++t(); 
    }
};

Imo we should diagnose this earlier if that is feasible. GCC also suffers from the same problem, which caused ++this as a typo for ++*this to go unnoticed in a libstdc++ header because the template containing it was never instantiated by libstdc++. MSVC also accepts this code. Is this a bug or a standards defect? Because I don’t see a situation where ++this would be semantically valid.

CC @AaronBallman, @cor3ntin, @erichkeane

[Sirraide]

More examples; these are even worse imo (compiles if S is dependent):

S s;
++s++;
s++++;

[Sirraide]

Actually, those could make sense if the postfix ++ operator is overloaded to return an lvalue, but if S is a pointer type then this should definitely be ill-formed.

[davidstone]

Overall, this seems like a good suggestion. Unfortunately, it's a bit more complicated than this, I think.

1595988 is a recent commit that partially added the behavior you are requesting (and is the reason you needed to add in the base class to get the code to get accepted). However, that change is what revealed the bug in libstdc++ when compiling with clang, which means that as of right now, clang cannot compile the most recently released version of libstdc++. I'm not really sure how we want to proceed based on that.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115119 is the associated libstdc++ bug.

[erichkeane]

Ooof, yeah, we probably DO have to accept the ++this in at least the libstdc++ case. I'd rather we do the absolute hackiest thing possible though.

One thought would be to do this as a 'Warn-as-default-error' for a while, so that 'system headers' don't catch it.

[shafik]

Worth noting, no implementation diagnoses this: https://godbolt.org/z/zssdnMe8G

I guess this is IFNDR, so diagnosing it eventually, is fine.

[Sirraide]

that change is what revealed the bug in libstdc++ when compiling with clang

Yeah, the libstdc++ bug is also also how I noticed this bug.

One thought would be to do this as a 'Warn-as-default-error' for a while

A warning would probably also work if you feel that making this an error would be too disruptive (for what it’s worth, I had to manually edit the libstdc++ header in question on my system to get things compiling again), but imo we should definitely issue some sort of diagnostic for this, because there is no situation where ++this isn’t a bug.

Worth noting, no implementation diagnoses this: https://godbolt.org/z/zssdnMe8G

Imo it’s worth pointing out though that the only reason the libstdc++ bug wasn’t caught earlier is because no-one diagnoses this.

I guess this is IFNDR, so diagnosing it eventually, is fine.

Just because I’m curious, where in the standard does it say that, because I only remember this always being a prvalue, but I’m not sure if that still applies if it is dependent.

[erichkeane]

A warning would probably also work if you feel that making this an error would be too disruptive (for what it’s worth, I had to manually edit the libstdc++ header in question on my system to get things compiling again), but imo we should definitely issue some sort of diagnostic for this, because there is no situation where ++this isn’t a bug.

I think we HAVE to here unfortunately, we can't really fail to compile past versions of libstdc++ for a long while.

Imo it’s worth pointing out though that the only reason the libstdc++ bug wasn’t caught earlier is because no-one diagnoses this.

100%

Just because I’m curious, where in the standard does it say that, because I only remember this always being a prvalue, but I’m not sure if that still applies if it is dependent.

Not going to look it up now, but ++this isn't ill-formed, NDR. Its that diagnosing in an uninstantiated template is NDR.

[Sirraide]

I think we HAVE to here unfortunately, we can't really fail to compile past versions of libstdc++ for a long while.

I don’t know how long this has been in libstdc++ (though it will be fixed as soon as version 1.14.2 is released), but yeah, that’s true; in that case it does seem like making it an error that’s a warning by default is probably the way to go.

Its that diagnosing in an uninstantiated template is NDR.

Ah, I see, that makes sense.

[erichkeane]

I lied, I looked it up:
https://eel.is/c++draft/temp#res.general-6

And referenced here: https://eel.is/c++draft/temp#inst-example-11

[Sirraide]

I lied, I looked it up: https://eel.is/c++draft/temp#res.general-6

And referenced here: https://eel.is/c++draft/temp#inst-example-11

Good to know. Thanks!