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.

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

Fix wrong parsing when a function call with a (one or more) variable … #327

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix wrong parsing when a function call with a (one or more) variable … #327

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
54 changes: 47 additions & 7 deletions corpus/statements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1264,15 +1264,55 @@ class A {
parameters: (parameter_list)
body: (block
(using_statement
(variable_declaration
(using_variable_declaration
type: (implicit_type)
(variable_declarator
(using_variable_declarator
name: (identifier)
(equals_value_clause
(identifier))))
body: (block
(return_statement))))))))

================================================================================
Using statement with function call
================================================================================

class A {
void Sample() {
var four = 4;
using (func(four)) {
return;
}
}
}

--------------------------------------------------------------------------------

(compilation_unit
(class_declaration
name: (identifier)
body: (declaration_list
(method_declaration
type: (predefined_type)
name: (identifier)
parameters: (parameter_list)
body: (block
(local_declaration_statement
(variable_declaration
type: (implicit_type)
(variable_declarator
name: (identifier)
(equals_value_clause
(integer_literal)))))
(using_statement
(invocation_expression
function: (identifier)
arguments: (argument_list
(argument
(identifier))))
body: (block
(return_statement))))))))

================================================================================
Using statement with compound variable declaration
================================================================================
Expand All @@ -1297,9 +1337,9 @@ class A {
parameters: (parameter_list)
body: (block
(using_statement
(variable_declaration
(using_variable_declaration
type: (identifier)
(variable_declarator
(using_variable_declarator
name: (identifier)
(equals_value_clause
(invocation_expression
Expand All @@ -1310,7 +1350,7 @@ class A {
(argument
(string_literal
(string_literal_fragment)))))))
(variable_declarator
(using_variable_declarator
name: (identifier)
(equals_value_clause
(object_creation_expression
Expand Down Expand Up @@ -1376,9 +1416,9 @@ class A {
parameters: (parameter_list)
body: (block
(using_statement
(variable_declaration
(using_variable_declaration
type: (identifier)
(variable_declarator
(using_variable_declarator
name: (identifier)
(equals_value_clause
(identifier))))
Expand Down
14 changes: 13 additions & 1 deletion grammar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ module.exports = grammar({

[$.tuple_element, $.declaration_expression],
[$.tuple_element, $.variable_declarator],
[$.tuple_element, $.using_variable_declarator],

[$.constant_pattern, $._name],
[$.constant_pattern, $._name, $._lvalue_expression],
Expand Down Expand Up @@ -1160,11 +1161,22 @@ module.exports = grammar({

unsafe_statement: $ => seq('unsafe', $.block),

using_variable_declaration: $ => seq(
field('type', $._type),
commaSep1($.using_variable_declarator)
),
Comment on lines +1164 to +1167
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using_variable_declaration could be reused in fixed_statement too, so I'd rename it to variable_declaration_no_tuple, or something similar.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd really prefer we keep the names aligned with Roslyn. Apart from the compatibility it really helps understanding how the grammar rules change given the ongoing evolution of C#.

Copy link
Author

Choose a reason for hiding this comment

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

So what name should I use?


using_variable_declarator: $ => seq(
field('name', $.identifier),
optional($.bracketed_argument_list),
Copy link
Collaborator

@tamasvajk tamasvajk Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need optional($.bracketed_argument_list)? Why was it added to variable_declarator in the first place?

Copy link
Author

Choose a reason for hiding this comment

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

I have no idea, unfortunately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@damieng Can you think of a case when we'd need the bracketed_argument_list in variable_declarator? I reran the tests without it, and everything passes.

optional($.equals_value_clause)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really optional to have an equals_value_clause in a using statement?

Copy link
Author

Choose a reason for hiding this comment

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

Again, I don't know.

),

using_statement: $ => seq(
optional('await'),
'using',
'(',
choice($.variable_declaration, $._expression),
choice($.using_variable_declaration, $._expression),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
choice($.using_variable_declaration, $._expression),
choice(alias($.using_variable_declaration, $.variable_declaration), $._expression),

')',
field('body', $._statement)
),
Expand Down
1 change: 1 addition & 0 deletions queries/highlights.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@

;; Variable declarations
(variable_declarator (identifier) @variable)
(using_variable_declarator (identifier) @variable)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we wouldn't need this if the alias was added.

(for_each_statement left: (identifier) @variable)
(catch_declaration (_) (identifier) @variable)

Expand Down
Loading