Warning for static member accessed through dot or arrow

I would like to propose a new warning for clang, and submit a patch implementing it. This would be my first patch submitted to LLVM. The warning would catch constructs like this:

QThread t;
t.sleep(1);

The casual reader (or even the writer) may assume this code causes the thread t to sleep, which is not the case. QThread::sleep is a static method, which causes the current thread to sleep. The code could be rewritten to clarify the meaning/intent:

QThread::sleep(1);
// or:
decltype(t)::sleep(1);

I do not propose enabling the warning by default, or including it in -Wall or -Wextra because of the large body of code in the wild that treats this construct as an idiomatic way to refer to static members. I propose the flag -Wunused-object-expression (“object expression” being the term used by the standard to denote the expression to the left of a member access operator).

I was quite surprised that none of the compilers or static analyzers I tried would diagnose this, even at their highest warning levels, which leads me to question whether I am missing something. Could there be a legitimate reason not to transform a member access of a static member into an id-expression as in the above example? I can think of two cases in which the warning should be inhibited: implicit this-> and when the staticness of the member could depend on a template parameter. If the object expression has some side effects that the user wants to preserve, they can always be moved to a separate statement.

Comments?

Thanks
Mat

In LibreOffice, we use a plugin to detect those (<https://cgit.freedesktop.org/libreoffice/core/tree/compilerplugins/clang/staticaccess.cxx>), for two reasons:

* There were cases where removing the redundant object expression made some variables or function parameters become unused, which allowed further code clean-up.

* In some cases MSVC indirectly warns about such redundant object expressions, when they involve otherwise unused function parameters (producing a warning about an unused parameter, which can be somewhat puzzling given the parameter /is/ used in the program text). So we could just as well warn about these things consistently.

On the other hand, these warnings are sometimes perceived as being on the border of "style police", so likely makes sense to have them only enabled explicitly.

Good to know. I went the route of implementing a compiler warning after I
read the following in <http://clang-analyzer.llvm.or
g/checker_dev_manual.html#ast>: "Some checks might not require
path-sensitivity to be effective. Simple AST walk might be sufficient. If
that is the case, consider implementing a Clang compiler warning."

So now the question is whether a plugin is satisfactory, or if there would
be sufficient benefit to having a warning available in the core compiler. I
believe there would be, but of course I am biased. The existence of a
plugin shows that there is at least one other user who desires this
feature, and that the concept has received a certain amount of testing
already. I think it meets the seven criteria at <
http://clang.llvm.org/get_involved.html>. The advantages of a warning
available in the vanilla compiler would be increased user reach, less
burdensome usage, exposure to more vigorous testing. I think the runtime
overhead would be negligible, as the diagnosis fits naturally into the
procedure for building a member access expression.

Maybe it's too simple an issue for anyone to waste time talking about, so
it might just be a case of submitting the patch and seeing what happens.
I'm just being extra cautious because it's my first patch to LLVM.
Coincidentally, this came up on StackOverflow just the other day: <
http://stackoverflow.com/q/41018973/1639256>

I don't have much to say on the overreach of the style police. Because of
the large body of user code that would trigger this warning, and which is
required by coding standards to compile cleanly with -Wall -Wextra it would
be disabled by default and not controlled by either of those flags. It
seems no more authoritarian than many existing warnings and I'm aware of no
potential false positives or justifiable leniency, except for cases I've
previously identified which can be easily filtered out.

(snip)

Maybe it's too simple an issue for anyone to waste time talking about, so it
might just be a case of submitting the patch and seeing what happens. I'm
just being extra cautious because it's my first patch to LLVM.
Coincidentally, this came up on StackOverflow just the other day:
<http://stackoverflow.com/q/41018973/1639256>

The fact that it caused somebody to go WTF when reading that code
suggests that a compiler warning about it *is* worthwhile.

That line of code probably caused a loss of 30-60 minutes of
programmer productivity.

Csaba

Playing devil's advocate against my own proposal for a moment, one /could/
argue that that example is a special case that is particularly confusing
because the member access of a static member is enabling a variable to be
referenced in its own initializer, and is therefore more deserving of a
warning than the general case.

Having said that, I do think it supports my general position that member
access of a static member is an obfuscation in any case.

I would like to propose a new warning for clang, and submit a patch
implementing it. This would be my first patch submitted to LLVM. The
warning would catch constructs like this:

    QThread t;
    t.sleep(1);

The casual reader (or even the writer) may assume this code causes the
thread t to sleep, which is not the case. QThread::sleep is a static
method, which causes the _current_ thread to sleep. The code could be
rewritten to clarify the meaning/intent:

    QThread::sleep(1);
    // or:
    decltype(t)::sleep(1);

I do not propose enabling the warning by default, or including it in
-Wall or -Wextra because of the large body of code in the wild that
treats this construct as an idiomatic way to refer to static members. I
propose the flag -Wunused-object-expression ("object expression" being
the term used by the standard to denote the expression to the left of a
member access operator).

I was quite surprised that none of the compilers or static analyzers I
tried would diagnose this, even at their highest warning levels, which
leads me to question whether I am missing something. Could there be a
legitimate reason not to transform a member access of a static member
into an id-expression as in the above example? I can think of two cases
in which the warning should be inhibited: implicit this-> and when the
staticness of the member could depend on a template parameter. If the
object expression has some side effects that the user wants to preserve,
they can always be moved to a separate statement.

Comments?

Sounds like a great candidate for a clang-tidy check (and possibly a fixup, too). Then you won't be held to quite as high standards w.r.t. false positives / false negatives as you would be with a clang warning. In addition, clang-tidy is a good "proving ground" for gathering data on FN's and FP's from large projects.

Cheers,

Jon

Not at all a derailment. This is a great counterexample. I would not write
code like this, but this is one of those cases where it can be idiomatic to
some people.