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
> >>>>>>
> >>>>>>
> >>>>
> >>>
> >>>
> >>
>
>

Reply via email to