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

Tree-sitter grammar: support if/else expressions #5343

Merged
merged 6 commits into from
Apr 3, 2024

Conversation

OceanOak
Copy link
Collaborator

@OceanOak OceanOak commented Apr 2, 2024

Changelog:

Tree-sitter-darklang
- Update the grammar to support if/else statements
- Add an external scanner to handle indentation

#5321

@OceanOak OceanOak force-pushed the grammar-updates-5.0 branch 2 times, most recently from a559e27 to 6512afe Compare April 2, 2024 14:07
@StachuDotNet
Copy link
Member

I wonder if we could write a function in our grammar.js that would abstract over this pattern

choice(
  seq(
    $.indent,
    repeat1(field("then_expression", $.expression)),
    $.dedent,
  ),
  field("then_expression", $.paren_expression),
),

like

let maybeWithIndentDedent (thing) = 
  choice(
    seq($.indent, thing, $.dedent),
    thing,
  ),

@OceanOak
Copy link
Collaborator Author

OceanOak commented Apr 2, 2024

I wonder if we could write a function in our grammar.js that would abstract over this pattern

choice(
  seq(
    $.indent,
    repeat1(field("then_expression", $.expression)),
    $.dedent,
  ),
  field("then_expression", $.paren_expression),
),

like

let maybeWithIndentDedent (thing) = 
  choice(
    seq($.indent, thing, $.dedent),
    thing,
  ),

Some initial thoughts:

  • The function may not recognize $.indent and $.dedent since they're defined after the function's declaration
    i.e.
let maybeWithIndentDedent ....

module.exports = grammar({
  name: "darklang",
  
  // I believe we can't declare that function here
  ...

so, the function would probably be more like

let maybeWithIndentDedent = (indent, thing, dedent) =>
  choice(seq(indent, thing, dedent), thing);
  • Losing the field names would make it challenging to capture in the parser, no?

Unrelated note:
we still need to wrap the 'then' expression in parentheses (only when writing an inline if/else expression) due to conflicts. planning to address this when fixing apply...

@OceanOak OceanOak force-pushed the grammar-updates-5.0 branch from 3b74d38 to 86d4295 Compare April 2, 2024 21:20
@OceanOak OceanOak marked this pull request as ready for review April 2, 2024 21:32
@OceanOak OceanOak requested a review from StachuDotNet April 2, 2024 21:42
repeat1(field("then_expression", $.expression)),
$.dedent,
),
field("then_expression", $.paren_expression),
Copy link
Member

Choose a reason for hiding this comment

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

it's neat that you needed a paren_expression here, but not in the equivalent else_expression below. This tree-sitter conflict stuff continues to be a mystery to me.

Comment on lines +318 to +331
("""if true then
a
else if false then
c
else if true then
d"""
|> roundtripCliScript) = """if true then
a
else
if false then
c
else
if true then
d"""
Copy link
Member

Choose a reason for hiding this comment

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

Since this PR is just focused on the parser, this is good -- could we follow this PR up with one focused on the pretty-printing of EIfs?
such that:
if an if has an else, whose root expression is an if as well, we format like:

if cond then
  expr
else if cond2 then
  expr2

?

Copy link
Member

Choose a reason for hiding this comment

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

(it might turn out that that code might need to 'track' indentation to do its job correctly)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I tried to make it work within this PR, but encountered some issues with the indentations

@StachuDotNet StachuDotNet merged commit 6262bea into darklang:main Apr 3, 2024
9 checks passed
@OceanOak OceanOak changed the title Update the grammar to support if/else statements Tree-sitter grammar: support if/else expressions Apr 3, 2024
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.

2 participants