Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix using p_kwnorest in f_no_kwarg #848

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

palkan
Copy link
Contributor

@palkan palkan commented Apr 5, 2022

Also, enable testing for 3.2 (that's why we haven't caught this yet).

p_kwnorest is resolved into s(:match_nil_pattern), which we can not use in f_no_kwarg.

Ref 820cc04

@iliabylich
Copy link
Collaborator

And then we should re-open #845, right? I guess correct fix is to change p_kwnorest rule to be reduced into [val[0], val[1]] and move s(:match_nil_pattern) construction to the place that needs it. From what I understand low-level p_kwnorest derivation constructs a node that is too high-level.

But I think it's ok to merge this PR and re-open a tracking issue.

@iliabylich
Copy link
Collaborator

Also, enable testing for 3.2 (that's why we haven't caught this yet).

my bad 👀

@palkan
Copy link
Contributor Author

palkan commented Apr 5, 2022

And then we should re-open #845, right?

Yep.

I think it's ok to merge this PR and re-open a tracking issue.

Let's wait a bit; I will try to provide a proper fix (as you suggested).

@palkan palkan changed the title Revert using p_kwnorest in f_no_kwarg Fix using p_kwnorest in f_no_kwarg Apr 5, 2022
@palkan
Copy link
Contributor Author

palkan commented Apr 5, 2022

@iliabylich Done!

@iliabylich iliabylich merged commit 1d275fc into whitequark:master Apr 5, 2022
@iliabylich
Copy link
Collaborator

@palkan Thank you

@palkan palkan deleted the fix/f-no-kwarg branch April 5, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants