Closed MarkCBell closed 5 years ago
Interesting! This is definitely a blind spot in our test suite, though I think it should be straightforward to fix. We just need better detection of the context of the *
operator: is it really a binary operator, is it part of extended function definition syntax, or is it unpacking? We only want to apply the ReplaceBinaryOperator
family of operators when it's really a binary operator.
@MarkCBell Could you try the fix-star-handling
branch and let me know if it works for you? I think it will address the problem you found.
So that does work for the specific example I had. However I think you also need to have star_expr
as a _NON_BINARY_PARENT to avoid examples such as:
def f(X):
a, *b = X
return a
from being mutated to
def f(X):
a, +b = X
return a
Are there any other cases? I don't know the python ast / parso that well.
I'm a bit on the fence about this one. From what I can tell, all of the mutations of unary-* result in invalid code which results in a kill, so cosmic-ray doesn't strictly need to avoid making these mutations. With that said, it's equally unnecessary to make these mutations in the first place for the same reason.
I think I'll add the check that you suggest just to avoid spending the time on these kinds of mutations. Thanks for pointing that out!
On Mon, May 27, 2019 at 11:16 AM Mark Bell notifications@github.com wrote:
So that does work for the specific example I had. However I think you also need to have star_expr as a _NON_BINARY_PARENT to avoid examples such as:
def f(X): a, *b = X return a
from being mutated to
def f(X): a, +b = X return a
Are there any other cases? I don't know the python ast / parso that well.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sixty-north/cosmic-ray/issues/443?email_source=notifications&email_token=AAATK6GG7FXJ4OJO4VSLQHTPXORG3A5CNFSM4HPXV5LKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWJIVCI#issuecomment-496142985, or mute the thread https://github.com/notifications/unsubscribe-auth/AAATK6HSF5MELNAEWFRTEALPXORG3ANCNFSM4HPXV5LA .
Ok, I've pushed up a change that has explicit handling of star_expr
. This feels like the right thing to do.
Are there any other cases? I don't know the python ast / parso that well.
That's a good question. I'm still learning my way around it myself. I've looked that their BNF/grammar definition, and I don't see any other obvious places where CR needs special handling.
The only other part of Python syntax that uses *
is from module import *
, so I'll see about adding handling for that...
Ok. I think that in that case parent.type
is import_from
so that might be another case that needs to be added to _NON_BINARY_PARENT
.
I agree. I've just pushed up a change to that effect.
Let me know if everything seems to be working from your perspective. If so, I'll merge in the branch and close this out.
Also, thanks for putting in the effort to sort this out. It makes a big difference for the project when users do this.
Yes, that all appears to be working for me. Thanks.
Fixed in 053d11a3e0554d0ce04b0ba5d80bb7850d2b2426
cosmic-ray incorrectly thinks that the '*' in functions like
are binary operators and so this operator mutates this to