metavariable-regex not working with pattern-inside?

[xmo-odoo]

When using pattern-inside and metavariable-regex which tries to filter one of the pattern-inside's metavariables, the match seems to never happen https://semgrep.dev/s/8ykq

  patterns:
    - pattern-either: 
      - pattern: |
          return None
      - pattern: |
          return
    - pattern-inside: |
        class $C:
            def $METHOD(...):
              ...
    - metavariable-regex:
        metavariable: "$METHOD"
        regex: "^[^_].*"

The intent is to match all methods which are "public" (not starting with _) and use a bare return or a return None. Commenting the metavariable-regex, all methods with the return are matched (including those we want to exclude).

Duplicating the pattern-inside so the metavariables are extracted from a pattern, things work as expected: https://semgrep.dev/s/gLGx

  patterns:
    - pattern-inside: |
        class $C:
            ...
    - pattern-either:
      - pattern: |
              def $METHOD(...):
                ...
                return
      - pattern: |
              def $METHOD(...):
                ...
                return None
    - metavariable-regex:
        metavariable: "$METHOD"
        regex: "^[^_].*"

The problem with the workaround is that I also want to match functions which don't return (and thus have an implicit return None), but I don't see how I could match a pattern-not: return $X inside a method with the proper name.

[mschwager]

Hey @xmo-odoo,

Thanks for bringing this to our attention, I'm going to investigate what's going on. This looks similar to #1699, hopefully we can solve this "metavariables with pattern-inside" in a generalized way 👍

[DrewDennison]

@xmo-odoo thanks for the bug report. Just checking what the priority of this fix is for you and if there are any timelines you're looking for a fix by?

[xmo-odoo]

@DrewDennison this is a low priority thing, I'm looking into semgrep and trying to use it personally (in part in order to better understand the quirks of pattern combinations, I don't think my mental model is correct yet) but that's about it.

This I decided to report because I spent a while trying to find out what I was doing wrong and a reduced test case showed it didn't seem to be supported (sometimes I can't get a rule to work properly in situ but I can't get the mismatch to trigger in a reduced test case, in those cases I rather assume I am doing something wrong somehow, even if I don't know what).

Maybe accessing a metavariable which doesn't exist should trigger an error or something? At least optionally? Or is there a tracing / debugging mode through which it'd be possible to see the flow of an item through a rule?

[xmo-odoo]

@mschwager

Thanks for bringing this to our attention, I'm going to investigate what's going on. This looks similar to #1699, hopefully we can solve this "metavariables with pattern-inside" in a generalized way +1

It does look like the same issue (just revealing itself in a different context). Should I close this one?

Maybe edit the title of #1699 so there's more chance people will find it when searching? Given #2154 it looks like I'm not the only one which missed the original.

[mschwager]

Hey @xmo-odoo,

After looking into this I understand what's going on. The patterns operator will only pass along metavariable information for findings that are present in all sub-operators. In other words, in your first example, since $METHOD is not present in your pattern operators its information will not be propagated to metavariable-regex.

The bad news is I don't see any code improvements we can make to get around this, the good news is we can create a rule that meets your needs, we just have to be crafty with our pattern-not: https://semgrep.dev/s/Zv34/.

More on why we can't make any code improvements: as mentioned above, metavariables are only propagated when they're present in all sub-operators. If this was not the case then patterns would propagate metavariables that don't meet our boolean logic specification, which would produce incorrect results.

Hope this is helpful! Let me know if this solves your problem and/or if there's anything else I can help with.

[xmo-odoo]

@mschwager thanks for the example, I'll take a look at it.

More on why we can't make any code improvements: as mentioned above, metavariables are only propagated when they're present in all sub-operators. If this was not the case then patterns would propagate metavariables that don't meet our boolean logic specification, which would produce incorrect results.

I understand the reasoning for pattern-either, but I can't grasp it for patterns (possibly because my mental model of semgrep remains incomplete or incorrect): since all branches of the pattern must match, surely it doesn't matter which branch matched it, and the metavariable will always match the logic specification?

[mschwager]

I understand the reasoning for pattern-either, but I can't grasp it for patterns (possibly because my mental model of semgrep remains incomplete or incorrect): since all branches of the pattern must match, surely it doesn't matter which branch matched it, and the metavariable will always match the logic specification?

Hmm, after thinking about it a bit more, I think you're correct that it makes sense to propagate metavariables in some situations. My initial thought is that this propagation makes sense when using patterns + pattern or pattern-inside, i.e. additive operations guaranteed to have equivalent metavariables under the conjunctive nature of patterns, but not things like pattern-either or filtering patterns (e.g. pattern-not, pattern-not-inside, etc.).

I'm trying to think through any obvious bugs that might occur with this change, but, again, I think we're okay due to the conjunctive nature of patterns. @brendongo does this seem reasonable? The nice thing here is bugs like #1699 and others relying on metavariable data will be solved in a more general nature with metavariable propagation.

I think the general flow would be something like:

• Only propagate metavariables when in patterns.
• Only propagate metavariables from additive pattern or pattern-inside sub-operators.
• Only propagate additional metavariables for a particular range, don't overwrite existing metavariables. I think this is a non-issue, since metavariable matching saves us, but I'd include this as a safety precaution to prevent unforeseen breakage.

Thoughts?

[brendongo]

Yeah propagating metavariables in patterns and pattern-inside but not in pattern-either makes sense.

[aryx]

Subscribe

[xmo-odoo]

@mschwager awesome

but not things like pattern-either or filtering patterns (e.g. pattern-not, pattern-not-inside, etc.).

That I've not used negative-filtering patterns much yet and thus didn't really think of them when I answered is probably why I had a much more optimistic view than you did. But glad to know it seems to work out in the end.

It'd make sense that only "positive" assertions "export" metavariables and negative ones don't, though from what you're writing it feels like semgrep uses metavariables more like unified terms so a negative assertion could theoretically bind a metavar if it was not bound by an other pattern?

[mschwager]

It'd make sense that only "positive" assertions "export" metavariables and negative ones don't, though from what you're writing it feels like semgrep uses metavariables more like unified terms so a negative assertion could theoretically bind a metavar if it was not bound by an other pattern?

Yeah the problem with negative assertion is there could be many things to "match" against. E.g.

pattern-not-inside: |
    def $FUNC(...):
        ...

If we're looking for a = ... in the following, what do we set $FUNC to?

a = 1

def func1():
    a = 2
    ...

def func2():
    a = 3
    ...

Generally I think it makes sense to propagate metavariables as outlined in my comment above, so I'll go ahead with the implementation 👍

[mschwager]

Hey @xmo-odoo, you're rule matches now! https://semgrep.dev/s/8ykq?version=0.42.0

[xmo-odoo]

@mschwager awesome! Thanks ❤️

[aryx]

I think there is no need to propagate metavariables during the match; we just need to AND the metavariable-regexp with the relevant patterns;
In some sense, metavariable-regexp is a trick to overcome some limitations of semgrep; we currently allow regexp in strings, so you don't do $E = "$STR" where metavariab-regexp: $STR regexp: foo|bar, but instead write directly $E = "=~/foo|bar"
so we just need to do the same; attach the metavariable-regexp to the relevant pattern as if it was part of the pattern.

[aryx]

I think this was a simple solution that didn't need any change to the engine: https://semgrep.dev/s/Doq2/ just group the metavariable-regexp with a nested patterns:

[mschwager]

I think this was a simple solution that didn't need any change to the engine: https://semgrep.dev/s/Doq2/ just group the metavariable-regexp with a nested patterns:

Not entirely sure why, but that rule errors out in v0.41.1: https://semgrep.dev/s/Doq2/?version=v0.41.1, so I'm not sure that would've fixed it.