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 >>>>>>>> >>>>> >>
signature.asc
Description: OpenPGP digital signature
