python.lang.maintainability.useless-inner-function false positives

[ievans]

https://semgrep.live/7KvR?registry=python.lang.maintainability.useless-innerfunction&sourceurl=https%3A%2F%2Fraw.githubusercontent.com%2Fdropbox%2Fdropbox-api-v2-repl%2Fa057a71995e12fb4cd2fff6fa4d917969e665fda%2Frepl.py

[underyx]

See also https://github.com/returntocorp/semgrep-app/pull/229/checks?check_run_id=890538919

[nbrahms]

Note that https://semgrep.live/YD8W/ would fix the rule.

[VeniVidiVici42]

This also fires on e.g.

def foo():
    def bar(n):
        return range(n)
    for i in bar(3):
        print(i)

which is not covered by the above proposed fix. It also fires on conditionals, which can be handled by adding

  - pattern-not-inside: |
      def $F(...):
          ...
          def $FF(...):
             ...
          ...
          if <...$FF...>:
             ...

but (what I think is) the loop equivalent

  - pattern-not-inside: |
      def $F(...):
          ...
          def $FF(...):
             ...
          ...
          for ... in <...$FF...>:
             ...

doesn't appear to catch this case. Playing around at https://semgrep.live/kbq1/.

[nbrahms]

@aryx It seems to me that deep-expression matching should apply to conditional expressions and generator expressions, non?

[aryx]

yes it should. Not sure what's going on. Maybe the generic AST for Python generators is wrong.

[aryx]

Can someone write a single pattern showing the problem?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[p4p3r]

@aryx not a single pattern, but probably the shortest I can write... https://semgrep.dev/s/KLDP

[aryx]

@brandonspark, want to have a look?

[brandonspark]

Here's my report from Slack: OK, the problem simply seems to be the fact that the pattern:

def $F(...):
  ...
  def $FF(...):
    ...
  ...

permits a match to the $F being the outermost function, and $FF being the innermost. This happens even though the indentation of the rule seems to suggest that the def $FF ought to occur at the top-level of the body of $F.

This messes things up because later when we discard ranges due to the pattern-not-inside, the discard doesn't go through, because the metavariables used in both sub-patterns do not match. To be honest, I don't know why we require that the metavariables must match.

You can see this demonstrated by the following playground link, where I've simply changed the names of the metavariables in the not-inside and thus no finding is produced: https://semgrep.dev/s/PL6e

The way I see it, there are three ways forward: Either we do not actually want the $FF to be able to match something at a deeper nested level, in which case we should change that, or we can change the requirement that metavariables match when discarding ranges, or we can live with the fact that this is how it works, and just use this renaming fix to fix the rule.

Open to suggestions on where to proceed next.