For Piotr's comment. IntelliJ does support adding/removing final parameters automatically.
You could even automatically add them on save with the Save Actions plugin [1]. [1] https://plugins.jetbrains.com/plugin/7642-save-actions On Mon, Oct 7, 2019 at 11:22 AM Piotr Nowojski <pi...@ververica.com> wrote: > 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 > >>>>>>>>> > >>>>>> > >>> > > > >
