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 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

Conversation

Sveder
Copy link

@Sveder Sveder commented Mar 17, 2024

…is inside a using statement. Instead of being parsed as invocation_expression it is being parsed as variable_declaration because of tuple_pattern.

…is inside a using statement. Instead of being parsed as invocation_expression it is being parsed as variable_declaration because of tuple_pattern.
@Sveder
Copy link
Author

Sveder commented Mar 17, 2024

Might be an acceptable fix for:
#326

@damieng damieng requested review from tamasvajk and removed request for tamasvajk April 7, 2024 09:27
@damieng
Copy link
Collaborator

damieng commented Apr 7, 2024

So while it solves one individual issue this breaks things by renaming variable_declaration to using_variable_declaration (and the same for declarators). Sometimes we'll take that break where we're better matching the Roslyn grammar/official C# syntax but they don't the using prefix either.

Best option here might be to see if you can keep the new using patterns but alias them so they don't have the using prefix (there are some examples in the grammar).

Comment on lines +1164 to +1167
using_variable_declaration: $ => seq(
field('type', $._type),
commaSep1($.using_variable_declarator)
),
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

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.

using_variable_declarator: $ => seq(
field('name', $.identifier),
optional($.bracketed_argument_list),
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),

@@ -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.

@Sveder
Copy link
Author

Sveder commented Apr 14, 2024

@damieng @tamasvajk First of all, thanks for taking a look. Much appreciated.
Second of all, some of your questions are somewhat over my head. I'm definitely not a C#/Roslyn expert (yet :)). I'd love some of your guidance in how to fix this better.

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.

3 participants