Consider splitting HidingField into two bugpatterns

[Flowdalic]

What version of Error Prone are you using?

2.2.0

Does this issue reproduce with the latest release?

yes

What did you do?

class Configuration {
}

class FooConfiguration extends Configuration {
}

class Super {
  protected Configuration foo;
}

class Sub extends Super {
  protected FooConfiguration foo;
}

While it is true that users of the Sub class can't interact with the 'foo' from the Super class, it is often not an issue if the field's type is a subtype of superclass's field's type. Maybe HidingField could be split into two bugpatterns, one where the field's type is a subtype, and one where not.

What did you expect to see?

No HidingField warning

What did you see instead?

A HidingField warning

[nick-someone]

In your example above,HidingField will actually not flag Sub.foo as hiding Super.foo, since Super.foo is private and not visible from Sub. I've written a test locally to confirm this, and that test should be mirrored out soon. If you're seeing something different, please let us know with a more direct replication.

The check would flag Sub.foo if Super.foo was protected or otherwise visible to Sub, since the creator of Super is assumed to have made foo visible to its subclasses for some reason (good Java practice is to keep instance members private by default, widening the visibility only as needed). In this circumstance, we think it's still important and/or interesting to understand that there are actually two different and visible foo fields present in the class, since the interaction between code in Super and code in Sub can lead to unintended consequences.

Filling out the example a bit more:

class Configuration {
  void setConfig(String key, String val) {}
  String getConfig(String key) {}
}

class FooConfiguration extends Configuration {
  void setFixedFoo(String value) { setConfiguration("foo", value); }
}

class Super {
  protected Configuration foo = new Configuration();
  void init() {} // Optional override
  String  go() {
     init();
     return foo.getConfig("foo");
  }
}

class Sub extends Super {
  private FooConfiguration foo = new FooConfiguration();
  void init() { foo.setFixedFoo("hi"); }
}

// Client code:
String value = new Sub().go();
// value is "" or whatever defaults from Configuration.getConfig, not "hi"

This is due to the declaration of Sub.foo. Without the declaration, use of foo in Sub targets Super.foo. Of course, in this example, you can't call setFixedFoo, but maybe that's actually appropriate, since you're not actually talking to a FooConfiguration.

[Flowdalic]

Sorry for the 'private', that should have been non-private. I've edited my post.

My use case/pattern is typically something like this

class Configuration {
}

class FooConfiguration extends Configuration {
}

class Super {
  protected final Configuration foo;
  protected Super(Configuration foo) {
    this.foo = foo;
  }
}

class Sub extends Super {
  protected final FooConfiguration foo; 
  protected Sub(FooConfiguration foo) {
    super(foo);
    this.foo = foo;
  }
}

Would be great if Error Prone could detect that both instances are identical in every case. But that appears to be not trivial.

[nick-someone]

Yeah, especially if Super is in a different compilation unit, we can't guarantee that's the case.

[delanym]

This use case is itself an error prone pattern https://refactoring.guru/smells/parallel-inheritance-hierarchies