Add support for CXXOperatorCallExpr in Expr::HasSideEffects

[Abpostelnicu]

Summary

As the there is support for BinaryOperator in order to see that it's of assignment type, this patch adds the same logic for the cases when assignment operators have been overloaded and used in different expressions.

For example:

class Number {
  int number;
public:
  Number& operator=(int a) {
    number = a;
    return *this;
  }
};

int main(int argc, const char * argv[]) {
  Number a;
  a = 2;
}

The AST node that will get generated for the assignment is of type CXXOperatorCallExpr so calling HasSideEffects on that expression would have resulted in a false return since CXXOperatorCallExpr was not considered to have a possible side effect when it's underlying operator type is assignment.

This patch addresses this exact issue and checks the underlying kind and if it's assignment it returns true, otherwise it continues the execution.

[aaron.ballman]

Thank you for the patch!

One thing this patch is missing is a test case that exercises this code path. For instance, there are diagnostics triggered for expressions with side effects when used as part of an unevaluated expression like sizeof, noexcept, etc. You should include a test case that demonstrates some behavioral change in the compiler that this patch addresses.

The AST node that will get generated for the assignment is of type CXXOperatorCallExpr so calling HasSideEffects on that expression would have resulted in a false return since CXXOperatorCallExpr was not considered to have a possible side effect when it's underlying operator type is assignment.

I think the underlying issue here is that HasSideEffects() accepts an argument as to whether possible side effects should count as definite side effects, and for the unevaluated context diagnostics, we do not want to include possible side effects, only definite ones. For instance, consider this very common code pattern where the function definition is not available to the TU:

struct S {
  S& operator=(int);
};

There's no way to know whether side effects will or won't happen for an assignment operator on an object of the above type because the definition does not exist. Assuming that side effects *definitely* happen, as your patch does, can then trigger false positives. So the second question is: how many false-positives will it generate? I think it may be reasonable to assume that operator=() has side effects, but you should run some large C++ projects (like LLVM itself) through Clang to see how many new diagnostics this change triggers and how many of those diagnostics are true positives vs false positives. That will tell us for sure whether this is adding value or not.

[Abpostelnicu]

In D22910#499082, @aaron.ballman wrote:

Thank you for the patch!

One thing this patch is missing is a test case that exercises this code path. For instance, there are diagnostics triggered for expressions with side effects when used as part of an unevaluated expression like sizeof, noexcept, etc. You should include a test case that demonstrates some behavioral change in the compiler that this patch addresses.

The AST node that will get generated for the assignment is of type CXXOperatorCallExpr so calling HasSideEffects on that expression would have resulted in a false return since CXXOperatorCallExpr was not considered to have a possible side effect when it's underlying operator type is assignment.

I think the underlying issue here is that HasSideEffects() accepts an argument as to whether possible side effects should count as definite side effects, and for the unevaluated context diagnostics, we do not want to include possible side effects, only definite ones. For instance, consider this very common code pattern where the function definition is not available to the TU:

struct S {
  S& operator=(int);
};

There's no way to know whether side effects will or won't happen for an assignment operator on an object of the above type because the definition does not exist. Assuming that side effects *definitely* happen, as your patch does, can then trigger false positives. So the second question is: how many false-positives will it generate? I think it may be reasonable to assume that operator=() has side effects, but you should run some large C++ projects (like LLVM itself) through Clang to see how many new diagnostics this change triggers and how many of those diagnostics are true positives vs false positives. That will tell us for sure whether this is adding value or not.

You are right, using this patch i've built the firefox codebase, witch is rather a very large product, and there is no issues triggered. I will also add some test cases.

[rsmith]

If IncludePossibleEffects is false, the goal of HasSideEffect is to determine whether we think the user intended for the expression to have a side-effect. It seems reasonable to assume that any use of an assignment operator had that intent, so (since it seems like this is not adding false positives) this seems reasonable to me.

lib/AST/Expr.cpp

2867–2868

Variable names should start with a capital letter.

2868

Please don't hard-code the order of OO enumerators like this (and this isn't even correct: you missed <<= and >>=).

Instead, consider extending BinaryOperator::isAssignmentOp / BinaryOperator::getOverloadedOpcode so you can use them for this.

2872

Please add a comment indicating the fallthrough here is intentional.

[Abpostelnicu]

lib/AST/Expr.cpp

2868

i was thinking more on doing for this specific case since BinaryOperator::isAssignmentOp and BinaryOperator::getOverloadedOpcode only accepts binary op codes and CXXOperatorCallExpr incapsulates operators both binary and unary

[rsmith]

lib/AST/Expr.cpp

2865–2867

Well, this is true for any overloaded operator. Perhaps something like

"When looking for potential side-effects, we assume that an overloaded assignment operator is intended to have a side-effect and other overloaded operators are not."

2869–2871

I think this should go after the check for a pure callee -- if for whatever reason someone has defined an operator= with __attribute__((pure)), we shouldn't claim it has a side-effect.

[Abpostelnicu]

I've added also support for pure in case CXXOperatorCallExprClass::isAssignmentOperator is true.

[aaron.ballman]

include/clang/AST/ExprCXX.h

109 ↗

(On Diff #69205)

Missing the full stop at the end of the sentence.

lib/AST/Expr.cpp

2869

Should this also handle overloaded ++ and -- if they're the prefix forms? What about overloaded operator new and delete (and the array forms)?

2872–2873

I think this is still missing the comment about fallthrough. Please use LLVM_FALLTHROUGH.

[Abpostelnicu]

lib/AST/Expr.cpp

2869

I think for now i should prefer only to have the implementation for assignment operator when it's overloaded.

[aaron.ballman]

The other thing this patch is missing are tests, btw.

lib/AST/Expr.cpp

2876

I think that operator++() and operator--() should be handled as well as the assignment operators. @rsmith, thoughts?

[sylvestre.ledru]

What about landing this version now (with test) and do the other operators in the a second commit? thanks

[aaron.ballman]

In D22910#549611, @sylvestre.ledru wrote:

What about landing this version now (with test) and do the other operators in the a second commit? thanks

I don't see a whole lot of value in that, but I may be missing information -- is this patch gating some other work? Supporting increment and decrement should be pretty straight-forward and is logically related to the current changeset. I'd prefer to fix it all at once since it shouldn't be overly onerous (and it protects us from accidentally only implementing part of the functionality), but if there's a reason to split it out, then I don't see harm in that either.

[Abpostelnicu]

i will add the unit tests in the next patch.

[malcolm.parsons]

lib/AST/Expr.cpp

2875

typo: increment

[Abpostelnicu]

corrected typo

[aaron.ballman]

This patch is missing tests for the new functionality.

include/clang/AST/ExprCXX.h

120 ↗

(On Diff #72806)

I'm not certain that isIncrementOp or isDecrementOp are useful predicate functions. I would test these conditions inside of Expr.cpp directly.

109 ↗

(On Diff #69205)

It looks like this comment has not been handled yet.

[Abpostelnicu]

Can someone please show me an example on how to write a test for this patch?

[aaron.ballman]

In D22910#601083, @Abpostelnicu wrote:

Can someone please show me an example on how to write a test for this patch?

You could test this with anything that diagnoses side effects in an unevaluated context (look for warn_side_effects_unevaluated_context diagnostics), such as use as the operand to sizeof.

lib/AST/Expr.cpp

     // These never have a side-effect.
     return false;
 
+  case CXXOperatorCallExprClass: {
+    // If it is an operator call expr it can have side effects when the
+    // underlaying operator is of assignment type
+    OverloadedOperatorKind binOp = cast<CXXOperatorCallExpr>(this)->getOperator();
+    if (binOp == OO_Equal || (binOp >= OO_PlusEqual && binOp <= OO_PipeEqual)) {
+      return true;
+    }
+  }
   case CallExprClass:
-  case CXXOperatorCallExprClass:
   case CXXMemberCallExprClass:
   case CUDAKernelCallExprClass:
   case UserDefinedLiteralClass: {