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

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1189440701-1784904942=:3487
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT



On Fri, 24 Jul 2026, Sang-Heon Jeon wrote:

> On Fri, Jul 24, 2026 at 3:45 AM Sang-Heon Jeon <[email protected]> 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 <[email protected]>
> > ---
> >  .../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

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
> >
>
--8323329-1189440701-1784904942=:3487--