Restructure Variable Parsing to support underscores #3412

bleaphar · 2024-01-12T18:56:43Z

Made a few changes around the parser design so that underscores are supported in the name of a variable and added two tests for binding and the underscores in the variable respectively.

shyamnamboodiripad · 2024-01-12T19:16:00Z

src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpVariableDeclarationNode.cs

@@ -17,7 +17,7 @@ internal HttpVariableDeclarationNode(SourceText sourceText, HttpSyntaxTree? synt
    {
    }

-    public string VariableName => ChildTokens.Where(t => t.Kind == HttpTokenKind.Word).Select(t => t.Text).FirstOrDefault() ?? string.Empty;
+    public string VariableName => string.Join("", ChildTokens.Where(t => t.Kind == HttpTokenKind.Word || (t.Kind == HttpTokenKind.Punctuation && t.Text is "_")).Select(t => t.Text)) ?? string.Empty;


Isn't this just this.Text?

https://github.com/dotnet/interactive/blob/main/src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpSyntaxNodeOrToken.cs#L39

Ah looks like the above node is for the entire declaration and not just the variable name. I think it may be better to have a dedicated sub-node for the variable name (which could potentially also be used to parse the variable references that appear inside {{}} expressions).

We could create a sub-node but I think the main utility would just be getting the value of the text and reconfiguring the parser to use this new node and I'm not sure that it is necessarily cleaner and worth the rework

I may be wrong, but I think we'll need to parse variable names once we start interpreting expressions and I think we may need to start doing that for the named requests feature (to implement support for this syntax for dotting into the named requests).

We may be able to sidestep this by using a separate / simpler (regex-based?) parser just for parsing the expression contents. Not sure if that is the path we will decide to go down though...

In any case, your call to keep this simple for now seems reasonable considering that we don't need to parse variable names in other places yet.

I don't think a regex is the right path. The different approaches within the same parser will make it harder to understand, and the regex isn't going to support language services as well.

+1 I had the same thoughts. Having a dedicated sub node could also make the implementation of features like go to definition / find all references simpler / more natural.

src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpRequestParser.cs

shyamnamboodiripad · 2024-01-12T20:28:31Z

src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpVariableDeclarationNode.cs

@@ -17,7 +17,7 @@ internal HttpVariableDeclarationNode(SourceText sourceText, HttpSyntaxTree? synt
    {
    }

-    public string VariableName => ChildTokens.Where(t => t.Kind == HttpTokenKind.Word).Select(t => t.Text).FirstOrDefault() ?? string.Empty;
+    public string VariableName => string.Join("", ChildTokens.Where(t => t.Kind == HttpTokenKind.Word || (t.Kind == HttpTokenKind.Punctuation && t.Text is "_")).Select(t => t.Text)) ?? string.Empty;


Should we report an error if a variable name contains characters like - that are not allowed? Perhaps this is not super urgent - but having dedicated parsing for variable names like we discussed above may make such error reporting easier.

This suggests an alternative approach, which is to parse everything but whitespace into the variable declaration node, and then report diagnostics for invalid characters.

Note that characters like - will be allowed on the RHS of the variable declaration (if they appear inside a string literal or embedded expression for example) which is why I think having dedicated parsing for the name portion may be helpful.

Ah never mind, I was confused by the naming. I assumed the HttpVariableDeclarationNode encompasses the entire declaration (including the = and the expression on the RHS). Looks like that is the HttpVariableDeclarationAndAssignmentNode - So please discount the above confusion from my comments above :)

(I also wonder if the naming could be made less confusing - but perhaps its ok since it was not as confusing once I looked at the names of all the involved node types :))

shyamnamboodiripad · 2024-01-12T20:32:24Z

src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpVariableDeclarationNode.cs

@@ -17,7 +17,7 @@ internal HttpVariableDeclarationNode(SourceText sourceText, HttpSyntaxTree? synt
    {
    }

-    public string VariableName => ChildTokens.Where(t => t.Kind == HttpTokenKind.Word).Select(t => t.Text).FirstOrDefault() ?? string.Empty;
+    public string VariableName => string.Join("", ChildTokens.Where(t => t.Kind == HttpTokenKind.Word || (t.Kind == HttpTokenKind.Punctuation && t.Text is "_")).Select(t => t.Text)) ?? string.Empty;


Looks like this could return the wrong thing if there happened to be an _ in the value portion of a variable declaration. Could you please add a test for that? (Again, I think having dedicated sub nodes for the variable name and value may make things easier.)

Ah never mind, I was confused by the naming. I assumed the HttpVariableDeclarationNode encompasses the entire declaration (inlcuding the = and the expression on the RHS). Looks like that is the HttpVariableDeclarationAndAssignmentNode :)

Since HttpVariableDeclarationNode only includes the name portion in addition to the @ prefix, perhaps we could just take this.Text (which will strip away any leading and trailing trivia) and then strip out the first @ character.

In other words, would something like below work?

Suggested change

public string VariableName => string.Join("", ChildTokens.Where(t => t.Kind == HttpTokenKind.Word || (t.Kind == HttpTokenKind.Punctuation && t.Text is "_")).Select(t => t.Text)) ?? string.Empty;

public string VariableName => this.Text.SubString(1);

jonsequitur · 2024-01-12T20:41:08Z

src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpRequestParser.cs

@@ -194,6 +194,10 @@ private HttpVariableDeclarationNode ParseVariableDeclaration()
                    {
                        ConsumeCurrentTokenInto(node);
                    }
+                    else if(CurrentToken is { Text: "_" })


Since these three branches perform the same action, you can combine them into this:

if (CurrentToken is { Text: "@" } or { Kind: HttpTokenKind.Word } or { Text: "_" }) { ConsumeCurrentTokenInto(node); }

bleaphar added 2 commits January 12, 2024 10:55

restructure to include underscores

89a7462

Merge branch 'dotnet:main' into main

8dc4565

shyamnamboodiripad reviewed Jan 12, 2024

View reviewed changes

src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpRequestParser.cs Show resolved Hide resolved

bleaphar added 2 commits January 12, 2024 11:44

Test for underscores in variable resolution

add686c

Merge branch 'main' of https://github.com/bleaphar/interactive

f95ab41

shyamnamboodiripad reviewed Jan 12, 2024

View reviewed changes

jonsequitur reviewed Jan 12, 2024

View reviewed changes

Consolidation

8e64e71

shyamnamboodiripad approved these changes Jan 12, 2024

View reviewed changes

Change for linux

5060128

jonsequitur merged commit e035978 into dotnet:main Jan 12, 2024
4 checks passed

colombod added the Area-HTTP label Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure Variable Parsing to support underscores #3412

Restructure Variable Parsing to support underscores #3412

bleaphar commented Jan 12, 2024 •

edited

Loading

shyamnamboodiripad Jan 12, 2024

shyamnamboodiripad Jan 12, 2024

bleaphar Jan 12, 2024

shyamnamboodiripad Jan 12, 2024

jonsequitur Jan 12, 2024

shyamnamboodiripad Jan 12, 2024

shyamnamboodiripad Jan 12, 2024 •

edited

Loading

jonsequitur Jan 12, 2024

shyamnamboodiripad Jan 12, 2024

shyamnamboodiripad Jan 12, 2024 •

edited

Loading

shyamnamboodiripad Jan 12, 2024

shyamnamboodiripad Jan 12, 2024 •

edited

Loading

shyamnamboodiripad Jan 12, 2024 •

edited

Loading

jonsequitur Jan 12, 2024

	public string VariableName => string.Join("", ChildTokens.Where(t => t.Kind == HttpTokenKind.Word \|\| (t.Kind == HttpTokenKind.Punctuation && t.Text is "_")).Select(t => t.Text)) ?? string.Empty;
	public string VariableName => this.Text.SubString(1);

Restructure Variable Parsing to support underscores #3412

Restructure Variable Parsing to support underscores #3412

Conversation

bleaphar commented Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

shyamnamboodiripad Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleaphar commented Jan 12, 2024 •

edited

Loading

shyamnamboodiripad Jan 12, 2024 •

edited

Loading

shyamnamboodiripad Jan 12, 2024 •

edited

Loading

shyamnamboodiripad Jan 12, 2024 •

edited

Loading

shyamnamboodiripad Jan 12, 2024 •

edited

Loading