Riverside-Software / sonar-openedge

CABL (Code Analyzer for ABL in SonarQube) - ABL ANTLR4 Parser
https://riverside-software.fr
GNU Lesser General Public License v3.0
63 stars 26 forks source link

Conditional where clause warning #1131

Closed ccecvb closed 4 months ago

ccecvb commented 5 months ago

Is there an existing rule to detect the conditional where clause construct in the code below.

def var lTreeNode as char no-undo.

lTreeNode = "A".

for each Customer where (if entry(1, lTreeNode) = "A" then true else Name begins entry(2, lTreeNode)):
    display CustNum Name.
end.

The code executes fine in oe12.2 but results in an error in 12.8 Entry 2 is outside the range of list A. (560)

The short circuit evaluation of the conditional is no longer working, I logged a tech support case for this regression. Regardless of the outcome of that support case, I would like to be able to flag the code as potentially wrong

Conditional expressions are not entirely wrong, following code works in both 12.2 and 12.8

def var lTreeNode as char no-undo.
lTreeNode = "A".

for each Customer where Name begins (if entry(1, lTreeNode) = "A" then "" else entry(2, lTreeNode)):
    display CustNum Name.
end.
gquerret commented 5 months ago

The engine can report potential issues in such query expressions, but there will be plenty of false positives. In this example, no issue will be raised, and it's very easy to spot that no issue will be raised, but the expression engine would need access to runtime data (which it doesn't have):

def var lTreeNode as char no-undo.
lTreeNode = "A,B".
for each Customer where (if entry(1, lTreeNode) = "B" then true else Name begins entry(2, lTreeNode)):
    display CustNum Name.
end.

Rules governing the execution of expressions will have to be found, it's funny that second example doesn't raise an issue.

movedoa commented 5 months ago

655 would eliminate this problem nicely 😜

gquerret commented 5 months ago

@movedoa That's right, but that may raise too many issues to be usable ! I can start with #655 and expand with this ticket.

ccecvb commented 5 months ago

@gquerret The difference between the first and the second example is that in the first case the condition on the record is actually in the else branch. The second example is a "clean" conditional expression that results in a value which is then used in the condition on the record.

ccecvb commented 5 months ago

Tech support reacted. The regression is expected behavior as per https://community.progress.com/s/article/both-sides-of-or-are-evaluated-in-12-4-where-clause-when-the-first-is-true.

Short-circuit evaluation never existed in this case, the errors were just not reported.

Following example from that page fails for the same reason

DEFINE VARIABLE cKey AS CHARACTER   NO-UNDO INIT "value".

/* work for IF clause */
IF cKey = "value" OR INTEGER(cKey) = 0 THEN
    MESSAGE "Success" VIEW-AS ALERT-BOX.

/* doesn't work for query */
DEFINE TEMP-TABLE ttTemp NO-UNDO
    FIELD iKey as INTEGER.

FOR EACH ttTemp
    WHERE cKey EQ "value" OR iKey EQ INTEGER(cKey):
    MESSAGE "Success" VIEW-AS ALERT-BOX.
END.

A rule matching the situation could also be part of the 12.2 - 12.8 migration set. The rule should probably warn for potential short circuit situations.

@gquerret Do you think it's feasible to create something on short notice for this ? I'd be happy to beta test this on our codebase. #655 could probably help, but I also think it would create a massive load of false positives.

My knowledge of the parser is very rusty, otherwise I'd create a routine to extract all where clauses from the application and grep though them

ccecvb commented 5 months ago

The rule won't be easy, following code works fine in 12.8

/* doesn't work for query */
DEFINE TEMP-TABLE ttTemp NO-UNDO
    FIELD iKey as character .

create ttTemp. ikey = "value".
create ttTemp. ikey = "12".
create ttTemp. ikey = "14".

FOR EACH ttTemp
    WHERE iKey EQ "value" OR INTEGER(iKey) > 12:
    display iKey.
END.

The key difference lies in

the evaluation of conditions that do not involve fields in the table being queried were performed well before this, hence the errors on conditions that cannot execute successfully; like INTEGER("hello")