SAP / abap-cleaner

ABAP cleaner applies 95+ cleanup rules to ABAP code at a single keystroke
Apache License 2.0
439 stars 48 forks source link

short dump in ATC after READ TABLE replacement #288

Closed FD1899 closed 6 months ago

FD1899 commented 6 months ago

Hi,

first of thanks a lot for this great plugin.

I recently updated to the latest version and now notice that a short dump occurs during the ATC run as soon as a READ TABLE ASSIGNING[...] statement is replaced.

It is an ASSERTION FAILED dump in LINE 22 of Method MOVE_VAR_EXT (CL_CI_TEST_SELECT_BIN_SEARCH). This method is called in ANALYZE_PROC of the same class.

       when 'ASSIGN'.
          read table <L_STMT>-TOKENS index 2 assigning <L_TOKEN>.
          L_VALUES = CHECK_VAR( P_TOKEN = <L_TOKEN> P_VARS = P_COLLECT-VARS ).
          if L_VALUES is not initial.
            read table <L_STMT>-TOKENS index 4 assigning <L_TOKEN>.
            MOVE_VAR_EXT( exporting P_TOKEN = <L_TOKEN> P_VALUES = L_VALUES P_CONDITION = L_CONDITION P_STMT = <L_STMT> P_PROGRAM = P_PROC-PROC_ID-PROGRAM
                          changing  P_VARS = P_COLLECT-VARS ).
          endif.
          continue.

Can you have a look at this?

We are on an ABAP 7.50 Release with EHP8. Please let me know if you need further information.

Thanks.

jmgrassau commented 6 months ago

Hi FD1899,

thanks for opening this. Hm, could you give an example of the (original and changed) READ TABLE / ASSIGN statement that triggers this issue? If the changed code compiles without errors, I wonder whether this could be an issue in CL_CI_TEST_SELECT_BIN_SEARCH itself (i.e. a syntax of the ASSIGN statement which is not correctly considered by this check)?

From what I can see in the documentation, the possible syntax of ASSIGN has not changed too much since ABAP 7.50 (except for the addition ELSE UNASSIGN and assignment of dynamic components { struc-(comp) } | { dref->(comp_name) } | { COMPONENT comp OF STRUCTURE struc }).

If I understand the check CL_CI_TEST_SELECT_BIN_SEARCH correctly, it searches for READ ... BINARY SEARCH statements which wrongly rely on the result of SELECT statements to be sorted; and doing that, it analyzes other statements on the way, including in this case ASSIGN statements. The CL_CI_TEST_SELECT_BIN_SEARCH->ANALYZE_PROC code seems similar up to ABAP 7.52, then changes with ABAP 7.53 (where ANALYZE_PROC is not redefined anymore by CL_CI_TEST_SELECT_BIN_SEARCH, and the inherited implementation does not have a specific section on ASSIGN statements).

Kind regards, Jörg-Michael

jmgrassau commented 6 months ago

Hi FD1899,

I just learned that the check CL_CI_TEST_SELECT_BIN_SEARCH is obsolete, including in ABAP 7.50. Instead, you can use CL_CI_TEST_NO_ORDER_BY ("Robust Programming -> Search problematic statements for result of SELECT/OPEN CURSOR without ORDER BY").

Kind regards, Jörg-Michael

FD1899 commented 6 months ago

Hi Jörg-Michael,

that's interesting. Can you give me the source for that information? So the reason for the dump is, that the ATC-check CL_CI_TEST_SELECT_BIN_SEARCH doesn't know the new syntax for reading internal tables? Unfortunately this test is still activated in our default check variant. So I have to talk the responsible person in my company.

Kind regards!

bjoern-jueliger-sap commented 6 months ago

Hi,

the source is me - I'm the product owner for the ABAP Test Cockpit and the Code Inspector at SAP.

Note that CL_CI_TEST_SELECT_BIN_SEARCH is assigned to the category of internal tests in SAP_BASIS release 7.50 (the folder in transaction SCI it's in is named "Internal Tests"). Checks in this category should not be used by customers to begin with (and you must have done some modifcation or workaround to even include a check from this category in your check variant).

The officially supported, non-internal successor of CL_CI_TEST_SELECT_BIN_SEARCH is CL_CI_TEST_NO_ORDER_BY. This newer check is superior in every respect.

FD1899 commented 6 months ago

Hi,

thank you very much. So we can close that issue here.

Kind Regards, FD