Bug: Rule "CREATE OBJECT -> NEW" causing short dump if static attributes are used

[frostrubin]

Hello & Good day,

we did a system wide cleanup and now have observed an error with the rule "Replace CREATE OBJECT with NEW constructor".

Reason:
CREATE OBJECT "instantiates" the object even before the CONSTRUCTOR has been run through/called.
NEW "instantiates" the object anonymously and only moves it to the correct memory area once it has been fully created.

This becomes apparent if a programmer decided to access static attributes via the instance instead of typing out the (usually longer) class name.

Example
Example Coding before cleanup:

   DATA go_docker TYPE REF TO cl_gui_docking_container.
   CREATE OBJECT go_docker
     EXPORTING
       side                    = go_docker->dock_at_bottom " Notice HERE how GO_DOCKER is used in its own creation
       ratio                   = 70
       no_autodef_progid_dynnr = 'X'.

Example Coding after cleanup:

   DATA go_docker TYPE REF TO cl_gui_docking_container.
   go_docker = NEW #(
       side                    = go_docker->dock_at_bottom
       ratio                   = 70
       no_autodef_progid_dynnr = 'X' ).

Error Behavior
The code after ABAP Cleaner run results in a short dump. The code before cleanup does not.

Suggested Fixes
Minimal:
Detect usage of own instance for static attribute access in statement CREATE OBJECT. Do not clean up.

Little-bit-higher:
Detect usage of own instance for static attribute access in statement CREATE OBJECT. Replace usage of static attribute with "proper" cl_class_name=>ac_attribute access. Then perform cleanup.

Even-bit-higher:
Re-use the coding created for option 2 and create a new rule that replaces all static attribute accesses that are done via instance with the "proper" static attribute access via class name.

Attention
A) If possible, the "minimal" fix should already accomodate usages that are more complex than simple "object named directly".

CREATE OBJECT LT_TABLE[ 1 ]-THE_REFERENCE 
  exporting
    iv_parameter = LT_TABLE[ 1 ]-THE_REFERENCE->ac_static attribute

So "everything between create object and parameter" instead of "just" first token.

B) This slight difference in compiler behavior also has at least one other side effect:
If the implementation of the CONSTRUCTOR already uses GO_DOCKER (instead of using ME->), it also works with CREATE OBJECT and does not work with NEW. However, I would consider this way more complex problem "out of scope" of this issue. But wanted to inform about it regardless.

[jmgrassau]

Hi Bernhard,

thanks a lot for opening this issue and for already putting so much thought into it! That's indeed an interesting difference between CREATE OBJECT and NEW.

For separation of concerns, I think we should go for the 'minimal' fix regarding "Replace CREATE OBJECT with NEW constructor" (by simply keeping CREATE OBJECT in these cases). Independently, it could be considered to add a new cleanup rule to "Replace Instance Access to Static Fields". If such a rule was run beforehand, it would automatically clean away the blocking issue, and "Replace CREATE OBJECT with NEW constructor" could then do its job.

However, unfortunately, such a new cleanup rule would not work in most cases, because to even determine which fields are static, ABAP cleaner would need to 'see' the class definition, and in most cases, it will be 'out of sight' (since ABAP cleaner does not make additional backend calls). That's even more true in complex cases such as LT_TABLE[ 1 ]-THE_REFERENCE->ac_static attribute.

Also, if we consider the following code:

CLASS cl_parent DEFINITION.
  PUBLIC SECTION.
    CLASS-DATA gc_parent_static TYPE i VALUE 1.
ENDCLASS.

CLASS cl_parent IMPLEMENTATION.
ENDCLASS.

CLASS cl_child DEFINITION INHERITING FROM cl_parent.
  PUBLIC SECTION.
    CLASS-DATA gc_child_static TYPE i VALUE 2.
    DATA mv_value TYPE i.
    METHODS constructor IMPORTING iv_value TYPE i.
ENDCLASS.

CLASS cl_child IMPLEMENTATION.
  METHOD constructor.
    super->constructor( ).
    mv_value = iv_value.
  ENDMETHOD.
ENDCLASS.

START-OF-SELECTION.
  DATA lo_child TYPE REF TO cl_child.

  CREATE OBJECT lo_child
    EXPORTING iv_value = lo_child->gc_parent_static.

Here, ABAP cleaner would even need to see the parent class definition. And then, should the last line be replaced with cl_child=>gc_parent_static or with cl_parent=>gc_parent_static? (That's a stylistic question – both is fine for the compiler).

So, not sure how far we'd get with such a cleanup rule. But "Replace CREATE OBJECT with NEW constructor" should definitely be fixed (including for complex cases)!

Regarding B): Yes, I agree, if the constructor itself access GO_DOCKER, that would be almost impossible to fix automatically. But this (hopefully theoretical) case is so much against any spirit of object-oriented programming that such a code sort of deserves to dump…

Kind regards,
Jörg-Michael

[jmgrassau]

Hi Bernhard,

here's what the 'minimal' fix can (and can't) do now:

Kind regards,
Jörg-Michael

[jmgrassau]

Hi Bernhard,

wait, these complex cases can't even happen:

CREATE OBJECT only allows for simple cases:

Kind regards,
Jörg-Michael

[frostrubin]

Hi Jörg-Michael,

thanks for the fast response and even faster fix! Looks good to me!

B) ... deserves to dump... That is my stance too.

Complex cases: Sorry for the confusion :(
I defenitely should have verified my own example first.
We had a finding/case where ls_struct-ref_name was used and that did lead me to the "don't just use the first token" caveat.

If your fix is integrated some time in the future, I am happy.

Thanks again for the fast help.
Best regards
Bernhard