After checking out the check style modules mentioned by Tison, I really do not see a point of enforcing/adding `final`. With ParameterAssignment [1] it’s redundant and will cause problems that I mentioned before.
Additionally enabling ParameterAssignment [1] would be probably much easier in our code base compared to enabling FinalParameters [2]. However still I’m not sure if that’s worth our troubles. I would be in favour of doing it only If enabling ParameterAssignment [1] doesn’t require many changes in our code base. Piotrek [1] https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment <https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment> [2] https://checkstyle.sourceforge.io/config_misc.html#FinalParameters <https://checkstyle.sourceforge.io/config_misc.html#FinalParameters> > On 7 Oct 2019, at 11:09, Dawid Wysakowicz <dwysakow...@apache.org> wrote: > > Hi all, > > My quick take on it. > > 1. If I were to introduce any rule on that I agree with Aljoscha, we > should rather enforce the `final` keyword rather than the opposite. > > 2. I think it does not make sense to enforce rules on such an > unimportant (in my opinion) topic. Generally I agree with Piotr that if > we want to introduce some rule we should have a way to automatically > enforce it. > > 3. I also agree with Piotr that problems with parameters reassignment > appear nearly exclusively in a very long methods. If we break long > methods in a shorter ones than there is usually no problem with a > parameter reassignment. > > 4. I would be ok with adding a point to our code style guidelines, but > honestly I am a bit skeptical. In the end we are not writing a book on > how to write a good software, but we rather list the most important > problems that we've seen so far in Flink code base and how to better > solve it. I'm not sure that's the case with the parameters reassignment. > > Best, > > Dawid > > On 07/10/2019 10:11, Zili Chen wrote: >> 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 >>>>>>>>> >>>>>> >>> >
