Skip to content

Reject p(p a, &block => value) and similar #3669

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+285 −38
Open

Reject p(p a, &block => value) and similar #3669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
255 changes: 218 additions & 37 deletions snapshots/command_method_call.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
@ ProgramNode (location: (1,0)-(41,10))
@ ProgramNode (location: (1,0)-(47,27))
├── flags: ∅
├── locals: [:foo, :bar]
└── statements:
@ StatementsNode (location: (1,0)-(41,10))
@ StatementsNode (location: (1,0)-(47,27))
├── flags: ∅
└── body: (length: 21)
└── body: (length: 24)
├── @ CallNode (location: (1,0)-(1,5))
│ ├── flags: newline, ignore_visibility
│ ├── receiver: ∅
Expand Down Expand Up @@ -737,39 +737,220 @@
│ │ ├── closing_loc: ∅
│ │ └── block: ∅
│ └── operator_loc: (39,7)-(39,9) = "or"
└── @ CallNode (location: (41,0)-(41,10))
├── flags: newline
├── receiver:
│ @ CallNode (location: (41,4)-(41,10))
│ ├── flags: ∅
│ ├── receiver:
│ │ @ CallNode (location: (41,5)-(41,10))
│ │ ├── flags: ignore_visibility
│ │ ├── receiver: ∅
│ │ ├── call_operator_loc: ∅
│ │ ├── name: :foo
│ │ ├── message_loc: (41,5)-(41,8) = "foo"
│ │ ├── opening_loc: ∅
│ │ ├── arguments:
│ │ │ @ ArgumentsNode (location: (41,9)-(41,10))
│ │ │ ├── flags: ∅
│ │ │ └── arguments: (length: 1)
│ │ │ └── @ IntegerNode (location: (41,9)-(41,10))
│ │ │ ├── flags: static_literal, decimal
│ │ │ └── value: 1
│ │ ├── closing_loc: ∅
│ │ └── block: ∅
│ ├── call_operator_loc: ∅
│ ├── name: :!
│ ├── message_loc: (41,4)-(41,5) = "!"
│ ├── opening_loc: ∅
│ ├── arguments: ∅
│ ├── closing_loc: ∅
│ └── block: ∅
├── @ CallNode (location: (41,0)-(41,10))
│ ├── flags: newline
│ ├── receiver:
│ │ @ CallNode (location: (41,4)-(41,10))
│ │ ├── flags: ∅
│ │ ├── receiver:
│ │ │ @ CallNode (location: (41,5)-(41,10))
│ │ │ ├── flags: ignore_visibility
│ │ │ ├── receiver: ∅
│ │ │ ├── call_operator_loc: ∅
│ │ │ ├── name: :foo
│ │ │ ├── message_loc: (41,5)-(41,8) = "foo"
│ │ │ ├── opening_loc: ∅
│ │ │ ├── arguments:
│ │ │ │ @ ArgumentsNode (location: (41,9)-(41,10))
│ │ │ │ ├── flags: ∅
│ │ │ │ └── arguments: (length: 1)
│ │ │ │ └── @ IntegerNode (location: (41,9)-(41,10))
│ │ │ │ ├── flags: static_literal, decimal
│ │ │ │ └── value: 1
│ │ │ ├── closing_loc: ∅
│ │ │ └── block: ∅
│ │ ├── call_operator_loc: ∅
│ │ ├── name: :!
│ │ ├── message_loc: (41,4)-(41,5) = "!"
│ │ ├── opening_loc: ∅
│ │ ├── arguments: ∅
│ │ ├── closing_loc: ∅
│ │ └── block: ∅
│ ├── call_operator_loc: ∅
│ ├── name: :!
│ ├── message_loc: (41,0)-(41,3) = "not"
│ ├── opening_loc: ∅
│ ├── arguments: ∅
│ ├── closing_loc: ∅
│ └── block: ∅
├── @ CallNode (location: (43,0)-(43,26))
│ ├── flags: newline, ignore_visibility
│ ├── receiver: ∅
│ ├── call_operator_loc: ∅
│ ├── name: :foo
│ ├── message_loc: (43,0)-(43,3) = "foo"
│ ├── opening_loc: (43,3)-(43,4) = "("
│ ├── arguments:
│ │ @ ArgumentsNode (location: (43,4)-(43,25))
│ │ ├── flags: ∅
│ │ └── arguments: (length: 1)
│ │ └── @ CallNode (location: (43,4)-(43,25))
│ │ ├── flags: ignore_visibility
│ │ ├── receiver: ∅
│ │ ├── call_operator_loc: ∅
│ │ ├── name: :bar
│ │ ├── message_loc: (43,4)-(43,7) = "bar"
│ │ ├── opening_loc: ∅
│ │ ├── arguments:
│ │ │ @ ArgumentsNode (location: (43,8)-(43,25))
│ │ │ ├── flags: contains_keywords
│ │ │ └── arguments: (length: 2)
│ │ │ ├── @ CallNode (location: (43,8)-(43,11))
│ │ │ │ ├── flags: variable_call, ignore_visibility
│ │ │ │ ├── receiver: ∅
│ │ │ │ ├── call_operator_loc: ∅
│ │ │ │ ├── name: :baz
│ │ │ │ ├── message_loc: (43,8)-(43,11) = "baz"
│ │ │ │ ├── opening_loc: ∅
│ │ │ │ ├── arguments: ∅
│ │ │ │ ├── closing_loc: ∅
│ │ │ │ └── block: ∅
│ │ │ └── @ KeywordHashNode (location: (43,13)-(43,25))
│ │ │ ├── flags: ∅
│ │ │ └── elements: (length: 1)
│ │ │ └── @ AssocNode (location: (43,13)-(43,25))
│ │ │ ├── flags: ∅
│ │ │ ├── key:
│ │ │ │ @ CallNode (location: (43,13)-(43,16))
│ │ │ │ ├── flags: variable_call, ignore_visibility
│ │ │ │ ├── receiver: ∅
│ │ │ │ ├── call_operator_loc: ∅
│ │ │ │ ├── name: :key
│ │ │ │ ├── message_loc: (43,13)-(43,16) = "key"
│ │ │ │ ├── opening_loc: ∅
│ │ │ │ ├── arguments: ∅
│ │ │ │ ├── closing_loc: ∅
│ │ │ │ └── block: ∅
│ │ │ ├── value:
│ │ │ │ @ CallNode (location: (43,20)-(43,25))
│ │ │ │ ├── flags: variable_call, ignore_visibility
│ │ │ │ ├── receiver: ∅
│ │ │ │ ├── call_operator_loc: ∅
│ │ │ │ ├── name: :value
│ │ │ │ ├── message_loc: (43,20)-(43,25) = "value"
│ │ │ │ ├── opening_loc: ∅
│ │ │ │ ├── arguments: ∅
│ │ │ │ ├── closing_loc: ∅
│ │ │ │ └── block: ∅
│ │ │ └── operator_loc: (43,17)-(43,19) = "=>"
│ │ ├── closing_loc: ∅
│ │ └── block: ∅
│ ├── closing_loc: (43,25)-(43,26) = ")"
│ └── block: ∅
├── @ CallNode (location: (45,0)-(45,26))
│ ├── flags: newline, ignore_visibility
│ ├── receiver: ∅
│ ├── call_operator_loc: ∅
│ ├── name: :foo
│ ├── message_loc: (45,0)-(45,3) = "foo"
│ ├── opening_loc: (45,3)-(45,4) = "("
│ ├── arguments:
│ │ @ ArgumentsNode (location: (45,4)-(45,25))
│ │ ├── flags: ∅
│ │ └── arguments: (length: 1)
│ │ └── @ CallNode (location: (45,4)-(45,25))
│ │ ├── flags: ignore_visibility
│ │ ├── receiver: ∅
│ │ ├── call_operator_loc: ∅
│ │ ├── name: :bar
│ │ ├── message_loc: (45,4)-(45,7) = "bar"
│ │ ├── opening_loc: ∅
│ │ ├── arguments:
│ │ │ @ ArgumentsNode (location: (45,8)-(45,25))
│ │ │ ├── flags: contains_keywords
│ │ │ └── arguments: (length: 2)
│ │ │ ├── @ CallNode (location: (45,8)-(45,11))
│ │ │ │ ├── flags: variable_call, ignore_visibility
│ │ │ │ ├── receiver: ∅
│ │ │ │ ├── call_operator_loc: ∅
│ │ │ │ ├── name: :baz
│ │ │ │ ├── message_loc: (45,8)-(45,11) = "baz"
│ │ │ │ ├── opening_loc: ∅
│ │ │ │ ├── arguments: ∅
│ │ │ │ ├── closing_loc: ∅
│ │ │ │ └── block: ∅
│ │ │ └── @ KeywordHashNode (location: (45,13)-(45,25))
│ │ │ ├── flags: ∅
│ │ │ └── elements: (length: 1)
│ │ │ └── @ AssocNode (location: (45,13)-(45,25))
│ │ │ ├── flags: ∅
│ │ │ ├── key:
│ │ │ │ @ ConstantReadNode (location: (45,13)-(45,16))
│ │ │ │ ├── flags: ∅
│ │ │ │ └── name: :KEY
│ │ │ ├── value:
│ │ │ │ @ CallNode (location: (45,20)-(45,25))
│ │ │ │ ├── flags: variable_call, ignore_visibility
│ │ │ │ ├── receiver: ∅
│ │ │ │ ├── call_operator_loc: ∅
│ │ │ │ ├── name: :value
│ │ │ │ ├── message_loc: (45,20)-(45,25) = "value"
│ │ │ │ ├── opening_loc: ∅
│ │ │ │ ├── arguments: ∅
│ │ │ │ ├── closing_loc: ∅
│ │ │ │ └── block: ∅
│ │ │ └── operator_loc: (45,17)-(45,19) = "=>"
│ │ ├── closing_loc: ∅
│ │ └── block: ∅
│ ├── closing_loc: (45,25)-(45,26) = ")"
│ └── block: ∅
└── @ CallNode (location: (47,0)-(47,27))
├── flags: newline, ignore_visibility
├── receiver: ∅
├── call_operator_loc: ∅
├── name: :!
├── message_loc: (41,0)-(41,3) = "not"
├── opening_loc: ∅
├── arguments: ∅
├── closing_loc: ∅
├── name: :foo
├── message_loc: (47,0)-(47,3) = "foo"
├── opening_loc: (47,3)-(47,4) = "("
├── arguments:
│ @ ArgumentsNode (location: (47,4)-(47,26))
│ ├── flags: ∅
│ └── arguments: (length: 1)
│ └── @ CallNode (location: (47,4)-(47,26))
│ ├── flags: ignore_visibility
│ ├── receiver: ∅
│ ├── call_operator_loc: ∅
│ ├── name: :bar
│ ├── message_loc: (47,4)-(47,7) = "bar"
│ ├── opening_loc: ∅
│ ├── arguments:
│ │ @ ArgumentsNode (location: (47,8)-(47,26))
│ │ ├── flags: contains_keywords
│ │ └── arguments: (length: 2)
│ │ ├── @ CallNode (location: (47,8)-(47,11))
│ │ │ ├── flags: variable_call, ignore_visibility
│ │ │ ├── receiver: ∅
│ │ │ ├── call_operator_loc: ∅
│ │ │ ├── name: :baz
│ │ │ ├── message_loc: (47,8)-(47,11) = "baz"
│ │ │ ├── opening_loc: ∅
│ │ │ ├── arguments: ∅
│ │ │ ├── closing_loc: ∅
│ │ │ └── block: ∅
│ │ └── @ KeywordHashNode (location: (47,13)-(47,26))
│ │ ├── flags: symbol_keys
│ │ └── elements: (length: 1)
│ │ └── @ AssocNode (location: (47,13)-(47,26))
│ │ ├── flags: ∅
│ │ ├── key:
│ │ │ @ SymbolNode (location: (47,13)-(47,17))
│ │ │ ├── flags: static_literal, forced_us_ascii_encoding
│ │ │ ├── opening_loc: (47,13)-(47,14) = ":"
│ │ │ ├── value_loc: (47,14)-(47,17) = "key"
│ │ │ ├── closing_loc: ∅
│ │ │ └── unescaped: "key"
│ │ ├── value:
│ │ │ @ CallNode (location: (47,21)-(47,26))
│ │ │ ├── flags: variable_call, ignore_visibility
│ │ │ ├── receiver: ∅
│ │ │ ├── call_operator_loc: ∅
│ │ │ ├── name: :value
│ │ │ ├── message_loc: (47,21)-(47,26) = "value"
│ │ │ ├── opening_loc: ∅
│ │ │ ├── arguments: ∅
│ │ │ ├── closing_loc: ∅
│ │ │ └── block: ∅
│ │ └── operator_loc: (47,18)-(47,20) = "=>"
│ ├── closing_loc: ∅
│ └── block: ∅
├── closing_loc: (47,26)-(47,27) = ")"
└── block: ∅
21 changes: 20 additions & 1 deletion src/prism.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14221,6 +14221,25 @@ parse_assocs(pm_parser_t *parser, pm_static_literals_t *literals, pm_node_t *nod
return contains_keyword_splat;
}

static inline bool
argument_allowed_for_bare_hash(pm_parser_t *parser, pm_node_t *argument) {
if (pm_symbol_node_label_p(argument)) {
return true;
}

switch (PM_NODE_TYPE(argument)) {
case PM_CALL_NODE: {
pm_call_node_t *cast = (pm_call_node_t *) argument;
if (cast->opening_loc.start == NULL && cast->arguments != NULL) {
return false;
}
break;
}
default: break;
}
return accept1(parser, PM_TOKEN_EQUAL_GREATER);
}

/**
* Append an argument to a list of arguments.
*/
Expand Down Expand Up @@ -14378,7 +14397,7 @@ parse_arguments(pm_parser_t *parser, pm_arguments_t *arguments, bool accepts_for
bool contains_keywords = false;
bool contains_keyword_splat = false;

if (pm_symbol_node_label_p(argument) || accept1(parser, PM_TOKEN_EQUAL_GREATER)) {
if (argument_allowed_for_bare_hash(parser, argument)){
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is very similar code here

prism/src/prism.c

Line 18217 in ce4abe1

if (pm_symbol_node_label_p(element) || accept1(parser, PM_TOKEN_EQUAL_GREATER)) {

I tried a few things to find code samples which force me to do the same change there as well but I did not find such code. Left it alone for now

if (parsed_bare_hash) {
pm_parser_err_previous(parser, PM_ERR_ARGUMENT_BARE_HASH);
}
Expand Down
41 changes: 41 additions & 0 deletions test/prism/errors/command_calls_35.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
p(p a, x: b => value)
^~ unexpected '=>'; expected a `)` to close the arguments
^ unexpected ')', expecting end-of-input
^ unexpected ')', ignoring it

p(p a, x: => value)
^~ unexpected '=>'; expected a `)` to close the arguments
^ unexpected ')', expecting end-of-input
^ unexpected ')', ignoring it

p(p a, &block => value)
^~ unexpected '=>'; expected a `)` to close the arguments
^ unexpected ')', expecting end-of-input
^ unexpected ')', ignoring it

p(p a, *args => value)
^~ unexpected '=>'; expected a `)` to close the arguments
^ unexpected ')', expecting end-of-input
^ unexpected ')', ignoring it

p(p a, **kwargs => value)
^~ unexpected '=>'; expected a `)` to close the arguments
^ unexpected ')', expecting end-of-input
^ unexpected ')', ignoring it

p p 1, &block => 2, &block
^~ unexpected '=>', expecting end-of-input
^~ unexpected '=>', ignoring it
^ unexpected ',', expecting end-of-input
^ unexpected ',', ignoring it
^ unexpected '&', ignoring it

p p p 1 => 2 => 3 => 4
^~ unexpected '=>', expecting end-of-input
^~ unexpected '=>', ignoring it

p[p a, x: b => value]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is code which I assumed would still be accepted with this change (see previous comment). But it is correctly rejected.

^ expected a matching `]`
^ unexpected ']', expecting end-of-input
^ unexpected ']', ignoring it

6 changes: 6 additions & 0 deletions test/prism/fixtures/command_method_call.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,9 @@ def foo = bar 1
!foo 1 or !bar 2

not !foo 1

foo(bar baz, key => value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably already tested somewhere 🤷


foo(bar baz, KEY => value)

foo(bar baz, :key => value)