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

[java] LawOfDemeter disallows method call on locally created object #3840

Closed
MetaBF opened this issue Mar 11, 2022 · 3 comments
Closed

[java] LawOfDemeter disallows method call on locally created object #3840

MetaBF opened this issue Mar 11, 2022 · 3 comments
Assignees
Labels
a:false-positive PMD flags a piece of code that is not problematic
Milestone

Comments

@MetaBF
Copy link

MetaBF commented Mar 11, 2022

Affects PMD Version: 6.43.0

Rule: LawOfDemeter

Please provide the rule name and a link to the rule documentation:
https://pmd.github.io/latest/pmd_rules_java_design.html#lawofdemeter

Description:

For many modern object oriented languages that use a dot as field identifier, the law can be stated simply as "use only one dot". That is, the code a.m().n() breaks the law where a.m() does not.

According to LawOfDemeter described in Wikipedia, the chain call should be warned. However, PMD failed to warn the chain call in get_nowarning() at point 1 (line 12).

As per #3840 (comment) below, the call chain is not detected, which is expected. However, moving the argument to allOf into a local variable produces a false positive, as shown at point 2 (line 19).

Code Sample demonstrating the issue:

import java.util.List;
import java.util.concurrent.CompletableFuture;

public class ParallelHandler<T> {

    private List<CompletableFuture<T>> futures;

    //...

    // pmd failed to warn
    public List<CompletableFuture<T>> get_nowarning() {
        CompletableFuture.allOf(futures.toArray(new CompletableFuture[] {})).join(); // point 1: true-negative
        return futures;
    }
    
    // pmd warns CompletableFuture.allOf(tempVar).join();
    public List<CompletableFuture<T>> get_fp() {
        CompletableFuture[] tempVar =  futures.toArray(new CompletableFuture[] {});
        CompletableFuture.allOf(tempVar).join(); // point 2: false positive
        return futures;
    }
}

Expected outcome:

PMD should report no violation at point 2 (line 19), but does. This is a false-positive.

Running PMD through: [CLI]

@MetaBF MetaBF added the a:false-positive PMD flags a piece of code that is not problematic label Mar 11, 2022
@MetaBF

This comment was marked as resolved.

@adangel adangel added a:false-negative PMD doesn't flag a problematic piece of code and removed a:false-positive PMD flags a piece of code that is not problematic labels Mar 11, 2022
@oowekyala
Copy link
Member

I don't think those are false negatives. Here you create a CompletableFuture using allOf, which produces a local object. If you can't call methods on local objects, then you can't do anything with them, and so you basically can't use the language. To me, your "true positive" is a false positive and should be removed.

More context: in PMD 7 the rule has been reimplemented and rationalized differently. Formulating LawOfDemeter as "use only one dot" is way too simplistic, as the rule is then so noisy that it is practically useless. The new rule instead says that you can only call methods on "trusted values", where eg, a formal parameter is trusted, fields of this are trusted, and an object created locally is trusted. Calling a getter on a "trusted value" produces an "untrusted value". The new rule only reports getter calls on untrusted values. This is closer to how Wikipedia defines the law of Demeter as "only talk to your friends" (here, trusted values are your friends), and significantly less noisy than "use only one dot".

In your example, the field futures is trusted. futures.toArray() is trusted as well, as it doesn't breach a boundary of abstraction, just converts futures into another format. The result of CompletableFuture.allOf is trusted, as it is an object created locally (allOf is a static construction method). That means you can call join on it without violation, and your example shouldn't report value code.

@MetaBF
Copy link
Author

MetaBF commented Mar 11, 2022

Thanks for your reply, I totally agree with you on

Formulating LawOfDemeter as "use only one dot" is way too simplistic, as the rule is then so noisy that it is practically useless.

Therefore it's a false-positive at point 2 (line 19)

@oowekyala oowekyala added a:false-positive PMD flags a piece of code that is not problematic and removed a:false-negative PMD doesn't flag a problematic piece of code labels Mar 11, 2022
@oowekyala oowekyala changed the title [java] PMD failed to detect LawOfDemeter violation [java] LawOfDemeter disallows method call on locally created object Mar 11, 2022
@oowekyala oowekyala added this to the 7.0.0 milestone May 2, 2023
@oowekyala oowekyala self-assigned this May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:false-positive PMD flags a piece of code that is not problematic
Projects
None yet
Development

No branches or pull requests

3 participants