Improve fix for #9570: check if ternary operator is used in assignment to reference

[chrchr-github]

No description provided.

[pfultz2]

It would be better to just check isVariableChanged on the ? token, so it can catch more cases such as passing it to a function(ie f(x?a:b)), modifying it in-place(ie (x?a:b)++), passing it to a for-range loop(ie for(int& e:(x?a:b))), and more. It also can be much simpler to write.

[chrchr-github]

Thanks, I'll try that.

This is all the comments I could find on isVariableChanged

/** Is variable changed by function call?
 * In case the answer of the question is inconclusive, e.g. because the function declaration is not known
 * the return value is false and the output parameter inconclusive is set to true
 *
 * @param tok           token of variable in function call
 * @param settings      program settings
 * @param inconclusive pointer to output variable which indicates that the answer of the question is inconclusive
 */

• there is no function call in this case
• ? is not a variable
• how is this discoverable at all?

[chrchr-github]

Using isVariableChanged(const Token *tok, int indirect, ...) seems to work, except there is a FN on the test case

void foo(bool flag) {
    bar( (flag) ? ~0u : ~0ul);
 }

isVariableChanged returns true because bar() might change something.
I have also tried the overload isVariableChanged(const Token *start, const Token *end, ...), but ran into some other issues.

[pfultz2]

? is not a variable

Yea that part is little confusing. Originally, this only worked for variables but now you can pass arbitrary expressions to it. We should probably rename it to isTokenChanged or something, but ultimately it would be better to annotate our token with this(so it doesnt need to run multiple times and addons can find out if something is modified).

isVariableChanged returns true because bar() might change something.

You can use the inconclusive flag to see if the result was inconclusive. We should probably change to the message to be inconclusive when this happens.

I have also tried the overload isVariableChanged(const Token *start, const Token *end, ...), but ran into some other issues.

Yea that is to check over a range of tokens, but it wont be useful for this.

[chrchr-github]

The "variable" is determined to be conclusively changed at https://github.com/danmar/cppcheck/blob/main/lib/astutils.cpp#L2188
Is the idea that the unknown bar() could take its argument by reference? In this case, that is impossible since the ternary contains only literals.
isVariableChanged() has more issues, see my other PR #3610.

[pfultz2]

The "variable" is determined to be conclusively changed at https://github.com/danmar/cppcheck/blob/main/lib/astutils.cpp#L2188

Ah thats right, this doesn't let you know if it was inconclusive or not. In ValueFlow, we call isVariableChangedByFunctionCall first to find out if its inconclusive and then call isVariableChanged here. We should probably add an optional bool* inconclusive parameter so we dont need to call it twice.

Is the idea that the unknown bar() could take its argument by reference?

Yes, or rather it could modify the result.

In this case, that is impossible since the ternary contains only literals.

Yea when indirect == 0 we should probably check if isConstVarExpression is true and return true in isVariableChanged. Although, we might need to extend isConstVarExpression to understand ternary operators.

isVariableChanged() has more issues, see my other PR #3610.

Yea that doesn't seem related.

[chrchr-github]

I'm getting confused here. For the test case #6426, isVariableChanged() called on ? returns true conclusively (i.e. inconclusive would be false even if we could check it), which leads to a FN. For the original case #9570, isVariableChanged returns true (presumably because of the reference on the lhs), which would fix the FP if we insert if (!isVariableChanged()... here.

Yea when indirect == 0 we should probably check if isConstVarExpression is true and return true in isVariableChanged.

Shouldn't this be "return false"? What exactly gets changed in this case?

[danmar]

but ultimately it would be better to annotate our token with this(so it doesnt need to run multiple times and addons can find out if something is modified).

sounds like a good idea 👍

[chrchr-github]

isVariableChanged() is probably not sufficient:

int a = 0, b = 0;
void f(bool b) {
    const int& x = b ? a : b;
    if (&x == getPtr()) {} // not changed, values still don't matter
}

[pfultz2]

isVariableChanged() called on ? returns true conclusively (i.e. inconclusive would be false even if we could check it),

So isVariableChangedByFunctionCall doesnt set inconclusive to true? That looks like a major bug in isVariableChangedByFunctionCall, as it should set inconclusive to true when there is no function definition.

isVariableChanged() is probably not sufficient:

Yea, I just wonder how common such a case is, especially after fixing the ValueFlow issue with globals/static variables. It seems odd to compare the address of global variables that are never modified that is picked among global variables that have the same value.

[pfultz2]

Shouldn't this be "return false"?

Yes, sorry was a typo.

[danmar]

So isVariableChangedByFunctionCall doesnt set inconclusive to true? That looks like a major bug in isVariableChangedByFunctionCall, as it should set inconclusive to true when there is no function definition.

I am not against it in theory. But could that lead to lots of false positives? I don't know if we've set a noise target for inconclusive but if we make this change and 99% of the new warnings are false positives then we should let it be as it is.

[pfultz2]

But could that lead to lots of false positives?

The FPs should be handled by the checks and ValueFlow analysis. isVariableChanged should just return if its being changed and if the information is incomplete/inconclusive(ie missing function definitions). Then checks or analysis can decide how to handle the inconclusive case to avoid FPs such as applying additional heuristics or making the warning inconclusive(if there isn't a high FP rate).

[danmar]

The FPs should be handled by the checks and ValueFlow analysis. isVariableChanged should just return if its being changed and if the information is incomplete/inconclusive(ie missing function definitions).

it sounds ok to me.

[chrchr-github]

isVariableChanged() called on ? returns true conclusively (i.e. inconclusive would be false even if we could check it),

So isVariableChangedByFunctionCall doesnt set inconclusive to true? That looks like a major bug in isVariableChangedByFunctionCall, as it should set inconclusive to true when there is no function definition.

I have checked again, and inconclusive is false, changed is true, I have circumvented this problem for now by checking isConstVarExpression().

isVariableChanged() is probably not sufficient:

Yea, I just wonder how common such a case is, especially after fixing the ValueFlow issue with globals/static variables. It seems odd to compare the address of global variables that are never modified that is picked among global variables that have the same value.

It's probably an edge case. Although global variables might be changed where cppcheck can't see it, or they might be extern volatile in the first place.

[pfultz2]

Although global variables might be changed where cppcheck can't see it, or they might be extern volatile in the first place.

Yes, and that is something that should be fixed in ValueFlow. We should only set the values to Known when we know conclusively the value can't be changed.

[pfultz2]

The CI failure has to do with regexes in matchcompiler.py, which is unrelated to this PR.

[danmar]

@pfultz2 if you approve this, I think it can be merged.

[chrchr-github]

Not strictly related to this PR, but anyway:
inconclusive is determined here:
const bool possiblyPassedByReference = (parenTok->next() == tok1 || Token::Match(tok1->previous(), ", %name% [,)}]"));
This returns false for parenTok = ( (opening parenthesis of the function call) and tok1= ? (cf. #3614 (comment)). Not sure what this is supposed to do in the normal case.

[pfultz2]

This returns false for parenTok = ( (opening parenthesis of the function call) and tok1= ? (cf. #3614 (comment)). Not sure what this is supposed to do in the normal case.

So that was added here, but this was before we used this function for arbitrary expressions. It probably needs to be updated, as I think its assuming its just a variable passed by reference.

Maybe we can open an issue to track this.

[chrchr-github]

I haven't been able to come up with an example that hinges on possiblyPassedByReference and ?. I found some other issues though...