VisibilityModifier: allow public final fields #2971

Closed
mtopolnik opened this Issue Feb 25, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@mtopolnik

mtopolnik commented Feb 25, 2016

VisibilityModifier check currently supports allowPublicImmutableFields, which allows a public field only if it satisfies all of the following:

  1. it is final;
  2. it is either of primitive type or of a reference type known to be immutable;
  3. its enclosing class is final.

The third constraint above reduces the usefulness of this relaxation because it cannot be applied to a design that asks for subclasses, even though a subclass can do nothing to jeopardize the immutability of the field that satisfies 1 and 2.

A discussion was held on this topic:
https://groups.google.com/forum/#!topic/checkstyle/r12blNy3ZVY

My point is quite simple: the option to allow public final fields should do so without further ado. The rest of the talk here was an attempt to show why the additonal restriction to final class is arbitrary and reduces usefulness of the option.

Example:

public class Chunk {
  public final long id;

  private int garbage; 
  ....
}

public class StableChunk extends Chunk {
  // inherits and uses id

  private final int size;
  ...
}

public class GrowingChunk extends Chunk {
  // inherits and uses id

  private int size;
  ...
}

In the above example the class Chunk is essentially non-final yet makes perfect use of a public final field. CheckStyle's current option does not allow me to codify this.

Roman Ivanov proposes a new property to the VisibilityModifier check: allowPublicFinalFields, which would only require that the field be final.

@romani romani added the approved label Feb 25, 2016

@som-snytt

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Mar 5, 2016

On my first day of trying to do checkstyle config on a project where checkstyle is used and beloved, I hit this issue.

The confusing part was that the final class condition is not prominent in the doc. I looked at the original commit; I see the current code says isImmutableFieldDefinedInFinalClass. OK.

Possible motivation for requiring final class is that the ctor might invoke a (protected, non-final) method before initing the field, so a subclass that implements or overrides the method can see the default value.

So the additional requirement says, We respect our contract for extension as much as our contract with other classes.

One way to express, "I will appear immutable to other classes and also to subclasses," is to require that the enclosing class is either final or listed in the known immutables.

That reduces the surface area of the config a bit.

However, it would also enable public final Encloser x = new MutableSubclass. Maybe that's OK.

I'll volunteer to fix this. Maybe I'll try out both approaches in two PRs and you can pick one.

On my first day of trying to do checkstyle config on a project where checkstyle is used and beloved, I hit this issue.

The confusing part was that the final class condition is not prominent in the doc. I looked at the original commit; I see the current code says isImmutableFieldDefinedInFinalClass. OK.

Possible motivation for requiring final class is that the ctor might invoke a (protected, non-final) method before initing the field, so a subclass that implements or overrides the method can see the default value.

So the additional requirement says, We respect our contract for extension as much as our contract with other classes.

One way to express, "I will appear immutable to other classes and also to subclasses," is to require that the enclosing class is either final or listed in the known immutables.

That reduces the surface area of the config a bit.

However, it would also enable public final Encloser x = new MutableSubclass. Maybe that's OK.

I'll volunteer to fix this. Maybe I'll try out both approaches in two PRs and you can pick one.

@mtopolnik

This comment has been minimized.

Show comment
Hide comment
@mtopolnik

mtopolnik Mar 5, 2016

We respect our contract for extension as much as our contract with other classes.

But put this into the perspective of the "solution": private field and getter. The subclass gets the exact same behavior. A rule that is orthogonal to a concern cannot pretend to be a rule that enforces it.

BTW don't the superclass initializers run first?

public class Test {
  final int a = 3;

  public static void main(String[] args) {
    new SubTest();
  }
}

class SubTest extends Test {
  SubTest() {
    System.out.println(a);
  }
}

This prints 3.

We respect our contract for extension as much as our contract with other classes.

But put this into the perspective of the "solution": private field and getter. The subclass gets the exact same behavior. A rule that is orthogonal to a concern cannot pretend to be a rule that enforces it.

BTW don't the superclass initializers run first?

public class Test {
  final int a = 3;

  public static void main(String[] args) {
    new SubTest();
  }
}

class SubTest extends Test {
  SubTest() {
    System.out.println(a);
  }
}

This prints 3.

@som-snytt

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Mar 5, 2016

The motivating example:

public class Test {
    final int a;

    public Test() {
        f();
        a = 42;
    }

    protected void f() { }

    public static void main(String... args) {
        new SubTest();
    }

    public static class SubTest extends Test {
        @Override protected void f() { System.out.println(a); }
    }
}

I understand your comment to mean that you can't stop people from doing really stupid stuff like this.

The modification to rule 3 would just say, OK, you can run this risk, but only if you promise not to by signing this document, i.e., adding yourself to immutables list.

I don't disagree with you. I was really trying to avoid two similarly-named properties, but maybe that's not a big deal.

The motivating example:

public class Test {
    final int a;

    public Test() {
        f();
        a = 42;
    }

    protected void f() { }

    public static void main(String... args) {
        new SubTest();
    }

    public static class SubTest extends Test {
        @Override protected void f() { System.out.println(a); }
    }
}

I understand your comment to mean that you can't stop people from doing really stupid stuff like this.

The modification to rule 3 would just say, OK, you can run this risk, but only if you promise not to by signing this document, i.e., adding yourself to immutables list.

I don't disagree with you. I was really trying to avoid two similarly-named properties, but maybe that's not a big deal.

@mtopolnik

This comment has been minimized.

Show comment
Hide comment
@mtopolnik

mtopolnik Mar 5, 2016

Yes, I thought you had in mind something like the above---accessing an overridable method from constructor. My main point was that the rule which is ostensibly about enforcing encapsulation cannot do anything here---the idiom remains broken in all cases. In fact, encapsulation makes it potentially worse because the rule won't stop you from overriding the getter and completely changing its semantics.

I agree with the concern of two similarly-named properties, but here CS can either change current semantics or introduce a new property. I don't know how important to backward compatibility is maintaining this detail about final class.

Yes, I thought you had in mind something like the above---accessing an overridable method from constructor. My main point was that the rule which is ostensibly about enforcing encapsulation cannot do anything here---the idiom remains broken in all cases. In fact, encapsulation makes it potentially worse because the rule won't stop you from overriding the getter and completely changing its semantics.

I agree with the concern of two similarly-named properties, but here CS can either change current semantics or introduce a new property. I don't know how important to backward compatibility is maintaining this detail about final class.

som-snytt added a commit to wallcode/checkstyle that referenced this issue Mar 9, 2016

Issue #2971: VisibilityModifier allowPublicFinalFields
Loosens the restriction for `allowPublicImmutableFields` that the
enclosing class must be final.

For `allowPublicFinalFields`, the field must be final and immutable.

The new property also defaults to true.

som-snytt added a commit to wallcode/checkstyle that referenced this issue Mar 10, 2016

som-snytt added a commit to wallcode/checkstyle that referenced this issue Mar 10, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 10, 2016

Member

The confusing part was that the final class condition is not prominent in the doc.

from html documentation , http://checkstyle.sourceforge.net/config_design.html#VisibilityModifier :
allowPublicImmutableFields - which allows immutable fields be declared as public if defined in final class.
Please let me know if there is a better way to let user know this nuance.

I don't know how important to backward compatibility is maintaining this detail about final class.

braking compatibility is very hard in UI plugins and extensions and .... . We try to keep compatibility as much as possible. New property need to be created. Documentation should be updated to have code examples to show how each property work.

Member

romani commented Mar 10, 2016

The confusing part was that the final class condition is not prominent in the doc.

from html documentation , http://checkstyle.sourceforge.net/config_design.html#VisibilityModifier :
allowPublicImmutableFields - which allows immutable fields be declared as public if defined in final class.
Please let me know if there is a better way to let user know this nuance.

I don't know how important to backward compatibility is maintaining this detail about final class.

braking compatibility is very hard in UI plugins and extensions and .... . We try to keep compatibility as much as possible. New property need to be created. Documentation should be updated to have code examples to show how each property work.

@som-snytt

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Mar 10, 2016

I don't know why I missed the final class when I read the doc the first time.

In the PR, the doc for the two properties differ by if defined in final class, so that actually highlights that info.

My concern about have two similarly-named properties is probably unfounded, since a project will decide once and for all which properties they want to disable.

I didn't add a doc example. I'll update with an example.

I don't know why I missed the final class when I read the doc the first time.

In the PR, the doc for the two properties differ by if defined in final class, so that actually highlights that info.

My concern about have two similarly-named properties is probably unfounded, since a project will decide once and for all which properties they want to disable.

I didn't add a doc example. I'll update with an example.

som-snytt added a commit to wallcode/checkstyle that referenced this issue Apr 18, 2016

som-snytt added a commit to wallcode/checkstyle that referenced this issue Apr 18, 2016

som-snytt added a commit to wallcode/checkstyle that referenced this issue May 3, 2016

MEZk added a commit to MEZk/checkstyle that referenced this issue May 25, 2016

MEZk added a commit to MEZk/checkstyle that referenced this issue Jun 1, 2016

MEZk added a commit to MEZk/checkstyle that referenced this issue Jun 2, 2016

romani added a commit that referenced this issue Jun 3, 2016

@romani romani added the new feature label Jun 3, 2016

@romani romani added this to the 7.0 milestone Jun 3, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 3, 2016

Member

fix is merged.

Member

romani commented Jun 3, 2016

fix is merged.

@romani romani closed this Jun 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment