Page MenuHomePhabricator

[AST Matchers] Fix bug in 'optionally' matcher wherein all previous bindings are cleared when all inner matchers fail.
ClosedPublic

Authored by ymandel on Feb 28 2020, 9:59 AM.

Details

Summary

The implementation of 'optionally' doesn't preserve bindings when none of the submatchers succeed. This patch adds a regression test for that behavior and fixes it.

Diff Detail

Event Timeline

ymandel created this revision.Feb 28 2020, 9:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2020, 9:59 AM
ymandel updated this revision to Diff 247299.Feb 28 2020, 10:00 AM

removed change to unrelated file.

Aaron -- when fixing this bug, I had some other questions about the behavior of this matcher. Specifically, instead of adding bindings to the existing map, it creates a new match result for each matching node (with addMatch). But, I'm struggling to see how it makes sense for optionally -- why would you want a separate match result for each matching InnerMatch? This behavior seems best reserved for forEach and related matchers.

I'd propose (and have a patch ready) to instead accumulate all bindings in the same result, so that optionally(a, b) would be equivalent to allOf(optionally(a), optionally(b)) (which it currently is not). That is, 'optionally' would always give a single result and each submatcher can add more bindings to the result.

Alternatively, we could restrict optionally to take exactly one argument, rather than making it variadic, in which case there's no room for confusion as to its semantics. Multiple optional elements would require using optionally within an allof (either explicit or implicit). However, I realize this change is more ambitious and may prevent some valid uses.

WDYT?

sbenza accepted this revision.Feb 28 2020, 10:18 AM
This revision is now accepted and ready to land.Feb 28 2020, 10:18 AM
aaron.ballman accepted this revision.Feb 28 2020, 10:38 AM

Patch LGTM aside from a formatting nit.

Aaron -- when fixing this bug, I had some other questions about the behavior of this matcher. Specifically, instead of adding bindings to the existing map, it creates a new match result for each matching node (with addMatch). But, I'm struggling to see how it makes sense for optionally -- why would you want a separate match result for each matching InnerMatch? This behavior seems best reserved for forEach and related matchers.

Agreed, that does seem a bit strange.

I'd propose (and have a patch ready) to instead accumulate all bindings in the same result, so that optionally(a, b) would be equivalent to allOf(optionally(a), optionally(b)) (which it currently is not). That is, 'optionally' would always give a single result and each submatcher can add more bindings to the result.

Alternatively, we could restrict optionally to take exactly one argument, rather than making it variadic, in which case there's no room for confusion as to its semantics. Multiple optional elements would require using optionally within an allof (either explicit or implicit). However, I realize this change is more ambitious and may prevent some valid uses.

WDYT?

Good question! My intuition is that optionally should take exactly one argument and the user should be explicit as to whether they mean allOf or anyOf when there is a list of optional matchers. Defaulting to the allOf behavior for a list may be surprising to some folks because the anyOf behavior also seems reasonable for a list of optional matchers. WDYT?

clang/lib/ASTMatchers/ASTMatchersInternal.cpp
353–354

Please fix the formatting.

ymandel updated this revision to Diff 247314.Feb 28 2020, 10:43 AM

fix formatting.

...

Good question! My intuition is that optionally should take exactly one argument and the user should be explicit as to whether they mean allOf or anyOf when there is a list of optional matchers. Defaulting to the allOf behavior for a list may be surprising to some folks because the anyOf behavior also seems reasonable for a list of optional matchers. WDYT?

Sounds good to me. That's my ideal solution. Will update my patch and send along... Thanks!

This revision was automatically updated to reflect the committed changes.

+1 to this fix.

However, regarding allOf vs. anyOf semantics, since optionally always succeeds, is there a difference between the two semantics?

It seems to me that there should be no difference between allOf(optionally(a), optionally(b)) vs. anyOf(optionally(a), optionally(b)).

+1 to this fix.

However, regarding allOf vs. anyOf semantics, since optionally always succeeds, is there a difference between the two semantics?

It seems to me that there should be no difference between allOf(optionally(a), optionally(b)) vs. anyOf(optionally(a), optionally(b)).

I think the difference is in whether you continue with the submatchers after a success. Allof does while anyof does not. That said, the original issue was forEach vs allOf/anyOf. So, I think Aaron's point holds - let optionally take one argument and then leave it to the user to explicitly specify forEach, allOf, anyOf, etc.

I think the difference is in whether you continue with the submatchers after a success. Allof does while anyof does not.

Oh, the short-circuiting makes a difference! I see.

+1 to making it non-variadic then.