Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False Positive: Omit self-reference when calling instance method #481

Open
zmsMarc opened this issue Oct 12, 2021 · 6 comments
Open

False Positive: Omit self-reference when calling instance method #481

zmsMarc opened this issue Oct 12, 2021 · 6 comments
Labels
style guide Clean ABAP related

Comments

@zmsMarc
Copy link

zmsMarc commented Oct 12, 2021

Check Name
Y_CHECK_SELF_REFERENCE

Actual Behavior
check reacts to usage of instance attributes when calling an instance method

  DATA(sum) = aggregate_values( me->values ).

Expected Behavior
Parameters should be irrelevant to the check

@zmsMarc zmsMarc added the bug Something isn't working correctly label Oct 12, 2021
@lucasborin
Copy link
Member

lucasborin commented Oct 13, 2021

The Clean ABAP discourages the use of self-reference. However, it is unclear if the rule is valid for instance methods and instance attributes, or methods only. In my understanding, we should avoid self-reference if it is not needed.

In your example, is there any local variable named values that could cause conflict?

@lucasborin lucasborin removed the bug Something isn't working correctly label Oct 13, 2021
@pokrakam
Copy link
Contributor

I am not sure why parameters should be irrelevant. That implies that these two statements should be treated differently:

result = some_method( me->value ). 
result = me->value.

They both should omit me-> unless there is a scope conflict.

@zmsMarc
Copy link
Author

zmsMarc commented Oct 13, 2021

I interpreted Clean ABAP that its superfluous for method calls as they can only ever be on the instance, however there is no mention of attributes/variables. IMO it would be better to have it explicitly mention that we want to use an attribute of the class.

I assume the test would still mark my example as an error even when I explicitly want to use an attribute instead of a local variable with the same name?

@lucasborin lucasborin added the style guide Clean ABAP related label Oct 13, 2021
@lucasborin
Copy link
Member

lucasborin commented Oct 13, 2021

I assume the test would still mark my example as an error even when I explicitly want to use an attribute instead of a local variable with the same name?

No, it wouldn't. The Check should identify that you have a local variable with the same name (scope conflict) and exempt it automatically.

@pokrakam
Copy link
Contributor

Oh dear, looks like we submitted PRs in parallel 😆
Ah well, let the stylists pick one

@lucasborin
Copy link
Member

No, it wouldn't. The Check should identify that you have a local variable with the same name (scope conflict) and exempt it automatically.

I have been updating the unit test based on this thread, and I figured out the instance attribute is out of the Check's scope. If the Clean ABAP team accepts one of our pull requests, I will update the Check accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style guide Clean ABAP related
Projects
None yet
Development

No branches or pull requests

3 participants