Re: [PATCH 01/36] coccinelle: misc: add cond_return_no_effect.cocci

[Sang-Heon Jeon]

On Fri, Jul 24, 2026 at 11:55 PM Julia Lawall <julia.lawall@xxxxxxxx> wrote:
>
>
>
> On Fri, 24 Jul 2026, Sang-Heon Jeon wrote:
>
> > On Fri, Jul 24, 2026 at 3:45 AM Sang-Heon Jeon <ekffu200098@xxxxxxxxx> wrote:
> > >
> > > Add a new Coccinelle script which removes a conditional return that
> > > has no effect:
> > >
> > >         if (ret)
> > >                 return ret;
> > >         return ret;
> > >
> > > Both branches return the same value, so the check can be removed.
> > > The condition can also be a negation or a comparison with a
> > > constant. Such code is usually a leftover from removing a
> > > statement between the two returns.
> > >
> > > When a local variable is assigned right before the check, the
> > > assignment and the two returns turn into a single return of the
> > > assigned expression, and the declaration is dropped if nothing
> > > else uses the variable. Otherwise only the check is removed.
> > >
> > > The fold can delete comments between the check and the final
> > > return, so the generated patch should be reviewed.
> > >
> > > Signed-off-by: Sang-Heon Jeon <ekffu200098@xxxxxxxxx>
> > > ---
> > >  .../misc/cond_return_no_effect.cocci          | 121 ++++++++++++++++++
> > >  1 file changed, 121 insertions(+)
> > >  create mode 100644 scripts/coccinelle/misc/cond_return_no_effect.cocci
> > >
> > > diff --git a/scripts/coccinelle/misc/cond_return_no_effect.cocci b/scripts/coccinelle/misc/cond_return_no_effect.cocci
> > > new file mode 100644
> > > index 000000000000..541aafdc32c6
> > > --- /dev/null
> > > +++ b/scripts/coccinelle/misc/cond_return_no_effect.cocci
> > > @@ -0,0 +1,121 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +///
> > > +/// Remove a conditional return that has no effect:
> > > +///
> > > +///    if (ret)
> > > +///            return ret;
> > > +///    return ret;
> > > +///
> > > +/// Both branches return the same variable, so the check has no
> > > +/// effect. It can also be a negation or a comparison with a
> > > +/// constant.
> > > +///
> > > +/// When a local variable is assigned right before the check, the
> > > +/// assignment and the two returns turn into a single return of the
> > > +/// assigned expression, and the declaration is dropped if nothing
> > > +/// else uses the variable. Otherwise only the check is removed.
> > > +///
> > > +// Such code is usually a leftover from removing a statement between
> > > +// the two returns.
> > > +//
> > > +// Confidence: High
> > > +// Copyright: (C) 2026 Sang-Heon Jeon
> > > +// Comments: The fold can delete comments between the check and the
> > > +//           final return, so review the generated patch.
> > > +// Options: --no-includes --include-headers
> > > +
> > > +virtual patch
> > > +virtual context
> > > +virtual org
> > > +virtual report
> > > +
> > > +//----------------------------------------------------------
> > > +//  For patch mode
> > > +//----------------------------------------------------------
> > > +
> > > +@collect depends on patch@
> > > +identifier ret;
> > > +expression E;
> > > +binary operator cmp = {<, <=, >, >=, ==, !=};
> > > +constant C;
> > > +@@
> > > +       ret = E;
> > > +       if (\(ret \| !ret \| ret cmp C\))
> > > +               return ret;
> > > +       return ret;
> > > +
> > > +@depends on patch@
> > > +local idexpression ret;
> > > +expression E;
> > > +binary operator cmp = {<, <=, >, >=, ==, !=};
> > > +constant C;
> > > +@@
> > > +-      ret = E;
> > > +-      if (\(ret \| !ret \| ret cmp C\))
> > > +-              return ret;
> > > +-      return ret;
> > > ++      return E;
> > > +
> >
> > If we can make these rules depend on an earlier rule, it should
> > improve the performance a bit. I will look into whether it is
> > worthwhile.
>
> I don't see an opportunity for more efficiency for the above rules, but go
> ahead if you have some idea.
>
> >
> > > +@depends on patch@
> > > +idexpression ret;
> > > +binary operator cmp = {<, <=, >, >=, ==, !=};
> > > +constant C;
> > > +@@
> > > +-      if (\(ret \| !ret \| ret cmp C\))
> > > +-              return ret;
> > > +       return ret;
> > > +
> > > +@depends on patch@
> > > +type T;
> > > +identifier collect.ret;
> > > +@@
> > > +-      T ret;
> > > +       ... when != ret
> > > +           when strict
> > > +
> > > +@depends on patch@
> > > +type T;
> > > +identifier collect.ret;
> > > +constant C;
> > > +@@
> > > +-      T ret = C;
> > > +       ... when != ret
> > > +           when strict
> > > +
> >
> > While testing the script, I found that this rule can unexpectedly
> > remove the declaration of a global or static variable in the same
> > file, even when it is still used elsewhere. There is no such case in
> > this series, but I will fix it in v2 by matching the declaration
> > inside a function.
>
> I think you could avoid matching the path from the top of the function to
> the declaration by doing the following:
>
> declaration D;
> statement S;
>
> (
> - T ret = C
> (
>   D
> |
>   S
> )
> &
>   T ret = C;
>   ... when != ret
>       when strict
> )
>
> The first part ensures that it is not a top-level declaration and the
> second part does the check you had previously.
>
> To ensure that it doesn't match "static T ret = C;" you can put the
> following in the @depends on patch@:
>
> disable optional_storage

Thanks for the suggestion. This looks much better than what I had in
mind. I'll use it in v2.

> julia
>
>
> > fn(...) {
> >       ...
> > -     T ret;
> >       ... when != ret
> >           when strict
> > }
> >
> > > +
> > > +//----------------------------------------------------------
> > > +//  For context mode
> > > +//----------------------------------------------------------
> > > +
> > > +@depends on context@
> > > +idexpression ret;
> > > +binary operator cmp = {<, <=, >, >=, ==, !=};
> > > +constant C;
> > > +@@
> > > +*      if (\(ret \| !ret \| ret cmp C\))
> > > +*              return ret;
> > > +       return ret;
> > > +
> > > +//----------------------------------------------------------
> > > +//  For org and report mode
> > > +//----------------------------------------------------------
> > > +
> > > +@r depends on org || report@
> > > +idexpression ret;
> > > +binary operator cmp = {<, <=, >, >=, ==, !=};
> > > +constant C;
> > > +position p;
> > > +@@
> > > +       if@p (\(ret \| !ret \| ret cmp C\))
> > > +               return ret;
> > > +       return ret;
> > > +
> > > +@script:python depends on org@
> > > +p << r.p;
> > > +@@
> > > +cocci.print_main("WARNING: conditional return with no effect (both branches return the same value)", p)
> > > +
> > > +@script:python depends on report@
> > > +p << r.p;
> > > +@@
> > > +coccilib.report.print_report(p[0], "WARNING: conditional return with no effect (both branches return the same value)")
> > > --
> > > 2.43.0
> > >
> >

Best Regards,
Sang-Heon Jeon