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

Simplify String rule in parser #2805

Merged
merged 1 commit into from
Aug 3, 2023
Merged

Conversation

emanuele6
Copy link
Member

@emanuele6 emanuele6 commented Aug 1, 2023

Use a StringStart component that is either FORMAT QQSTRING_START or QQSTRING_START instead of having two similar rules for String.

This is simpler and avoids having to use an untyped mid-rule action component to copy FORMAT at the top of the stack before QQString, and having to use jv_free($<literal>3) instead of jv_free($1) just to make bison not complain about the "unused" mid-rule component.

Use a StringStart component that is either FORMAT QQSTRING_START or
QQSTRING_START instead of having two similar rules for String.

This is simpler and avoids having to use an untyped mid-rule action
component to copy FORMAT at the top of the stack before QQString, and
having to use jv_free($<literal>3) instead of jv_free($1) just to make
bison not complain about the "unused" mid-rule component.
@itchyny
Copy link
Contributor

itchyny commented Aug 2, 2023

My first thought of skimming the rules is that including the format prefix in String is an unintended accident. .@html, {@html:.} are invalid but .@html"", {@html"":.} are valid. I think better rule is Term := String | Format | Format String | etc, which disallows unexpected formatted string usages around the syntax.

@emanuele6
Copy link
Member Author

emanuele6 commented Aug 3, 2023

Why should you only be allowed to use a different format for a string literal in an expression, and not when using a string literal in a object constructor as key, or as ."string", or as path for import/include? What is the point of forcing the user to use "\(@foo "blah")" instead of just @foo "blah" if they want to do that?
In python, and javascript, you can use a f'' or `` string interpolation wherever you can use a string literal.

@nicowilliams
Copy link
Contributor

I like this. It's easier to understand.

@nicowilliams
Copy link
Contributor

LGTM. I really like this simplification. I'm not seeing what it might break either.

My first thought of skimming the rules is that including the format prefix in String is an unintended accident. .@html, {@html:.} are invalid but .@html"", {@html"":.} are valid. I think better rule is Term := String | Format | Format String | etc, which disallows unexpected formatted string usages around the syntax.

.@html, {@html:.} remain invalid with this patch, and .@html"", {@html"":.} remain valid with this patch. Today I learned that the format specifier is an integral part of the string literal, and... I think it makes sense.

So I think this is ready, unless there's any other objections. Changing nothing in the language while simplifying the parser is a win.

Copy link
Contributor

@itchyny itchyny left a comment

Choose a reason for hiding this comment

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

Okay. Looks good.

@itchyny itchyny merged commit dcaf701 into jqlang:master Aug 3, 2023
28 checks passed
@emanuele6 emanuele6 deleted the simplestringstart branch August 3, 2023 23:42
@nicowilliams
Copy link
Contributor

Thanks!

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