Thanks for your thoughts Arvid & Piotr, I check out the effect of ParameterAssignment[1] and it does prevent codes from modifying parameter which I argued above the most value introduced by `final` modifier of parameter.
So firstly, I think it's good to enable ParameterAssignment in our codebase. Besides, there is no rule to forbid `final` modifier of parameter. Instead, there is a rule to enforce `final` modifier[2] but given [1] enabled it is actually redundant. The main purpose for, given enforced not modify parameters, reducing `final` modifiers of parameter is to remove redundant modifier so that we don't see have declaration like T fn( final ArgumentType1 argument1, final ArgumentType2 argument2, ...) because we actually don't mark final everywhere so it might make some confusions. Given [1] is enforced these `final` are indeed redundant so that we can add a convention to reduce `final` modifiers of parameters, which is a net win. Best, tison. [1] https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment [2] https://checkstyle.sourceforge.io/config_misc.html#FinalParameters Piotr Nowojski <pi...@ververica.com> 于2019年10月7日周一 下午3:49写道： > Hi, > > Couple of arguments to counter the proposal of making the `final` keyword > obligatory. Can we prepare a code style/IDE settings to add it > automatically? If not, I would be strongly against it, since: > > - Intellij’s automatic refactor actions will not work properly. > - I don’t think it’s a big deal. I don’t remember having any issues with > the lack or presence of the `final` keyword. > - `final` is pretty much useless in most of the cases (it’s not `const` > and it works only for the primitive types). > - I don’t like the extra overhead of doing something of very little extra > value. Especially the extra hassle of going back & forth during the reviews > (both as a contributor & as a reviewer). > - If missing `final` keyword caused some confusion, because surprisingly a > parameter was modified somewhere in the function and it wasn’t obviously > visible, the function is doing probably too many things and it’s too > long/too complicated… > > Generally speaking, I’m against adding minor things to our codestyle that > can not be enforced and added automatically. > > Piotrek > > > On 7 Oct 2019, at 09:13, Arvid Heise <ar...@ververica.com> wrote: > > > > Hi guys, > > > > I'm a bit torn. In general, +1 for making parameters effectively final. > > > > The question is how to enforce it. We can make it explicit (like Aljoscha > > said). All IDEs will show immediately warnings/errors for violations. It > > would allow to softly migrate code. > > > > Another option is to use a checkstyle rule [1]. Then, we could omit the > > final keyword and rely on checkstyle checks as we do for quite a few > other > > things. A hard checkstyle rule would probably fail on a good portion of > the > > current code base. But we would also remove reassignment to parameters > > (which I consider an anti-pattern). > > > > If we opt not to enforce it, then -1 for removing final keywords from my > > side. > > > > Best, > > > > Arvid > > > > [1] > > > https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/ParameterAssignmentCheck.html > > > > On Fri, Oct 4, 2019 at 1:22 PM Zili Chen <wander4...@gmail.com> wrote: > > > >> Hi Aljoscha, > >> > >> I totally agree with you on field topic. Of course it makes significant > >> difference whether > >> or not a field is final and IDE/compiler can help on checking. > >> > >> Here are several thoughts about final modifier on parameters and why I > >> propose this one: > >> > >> 1. parameters should be always final > >> > >> As described above, there is no reason a parameter to be non-final. So > >> different with field, > >> a field can be final or non-final based on whether or not it is > immutable. > >> Thus with such a > >> code style guide in our community, we can work towards a codebase every > >> parameter is > >> effectively final. > >> > >> 2. parameter final cannot be inherited > >> > >> Unfortunately Java doesn't keep final modifier of method parameter when > >> inheritance. So > >> even you mark a parameter as final in an interface or super class, you > have > >> to re-mark it > >> as final in its implementation or subclass. From another perspective, > final > >> modifier of > >> parameter is a local attribute of parameter so that we can narrow > possible > >> effect during > >> review. > >> > >> 3. IDE can lint difference between effective final and mutable parameter > >> > >> It is true that IDE such as Intellij IDEA can lint difference between > >> effective final and > >> mutable parameter(with underline). So that with this code style what we > >> lose is that > >> we cannot get a compile time error if someone modifies parameter in the > >> method body. > >> But as mentioned in 1, by no means we allow a parameter to be modified. > If > >> we agree > >> on this statement, then we hopefully converge in a codebase that no > >> parameter is > >> modified. > >> > >> For what we gain, I'd like to recur our existing code style of @Nonnull > to > >> be default. > >> Actually it does help for compiler to report compile time warning if we > >> possibly pass a > >> nullable value to an non-null field. We make @Nonnull as default to > "reduce > >> code noise" > >> so I think we can reuse the statement here also. > >> > >> Best, > >> tison. > >> > >> > >> Aljoscha Krettek <aljos...@apache.org> 于2019年10月4日周五 下午5:58写道： > >> > >>> I actually think that doing this the other way round would be correct. > >>> Removing final everywhere and relying on humans to assume that > everything > >>> is final doesn’t seem maintainable to me. The benefit of having final > on > >>> parameters/fields is that the compiler/IDE actually checks that you > don’t > >>> modify it. > >>> > >>> In general, I think that as much as possible should be declared final, > >>> including fields and parameters. > >>> > >>> Best, > >>> Aljoscha > >>> > >>>> On 2. Oct 2019, at 13:31, Piotr Nowojski <pi...@ververica.com> wrote: > >>>> > >>>> +1 from my side. > >>>> > >>>>> On 2 Oct 2019, at 13:07, Zili Chen <wander4...@gmail.com> wrote: > >>>>> > >>>>> Yes exactly. > >>>>> > >>>>> > >>>>> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道： > >>>>> > >>>>>> Hi Tison, > >>>>>> > >>>>>> To clarify your proposal. You are proposing to actually drop > the > >>>>>> `final` keyword from the parameters and we should implicilty assume > >>> that > >>>>>> it’s always there (in other words, we shouldn’t be modifying the > >>>>>> parameters). Did I understand this correctly? > >>>>>> > >>>>>> Piotrek > >>>>>> > >>>>>>> On 1 Oct 2019, at 21:44, Zili Chen <wander4...@gmail.com> wrote: > >>>>>>> > >>>>>>> Hi devs, > >>>>>>> > >>>>>>> Coming from this discussion[1] I'd like to propose that in Flink > >>> codebase > >>>>>>> we suggest a code style > >>>>>>> that parameters of method are always final. Almost everywhere > >>> parameters > >>>>>> of > >>>>>>> method are final > >>>>>>> already and if we have such consensus we can prevent redundant > final > >>>>>>> modifier in method > >>>>>>> declaration so that we survive from those noise. > >>>>>>> > >>>>>>> Here are some cases that might require to modify a parameter. > >>>>>>> > >>>>>>> 1. to set default; especially if (param == null) { param = ... } > >>>>>>> 2. to refine parameter; it is in pattern if ( ... ) { param = > >>>>>>> refine(param); } > >>>>>>> > >>>>>>> Either of the cases we can move the refine/set default logics to > the > >>>>>> caller > >>>>>>> or select another > >>>>>>> name for the refined value case by case. > >>>>>>> > >>>>>>> Looking forward to your feedbacks :-) > >>>>>>> > >>>>>>> Best, > >>>>>>> tison. > >>>>>>> > >>>>>>> [1] > https://github.com/apache/flink/pull/9788#discussion_r329314681 > >>>>>> > >>>>>> > >>>> > >>> > >>> > >> > >
