gdamore / tree-sitter-d

D Grammar for Tree Sitter
MIT License
41 stars 7 forks source link

Rename `declaration` to `definition` to match namings of other grammars? #16

Closed nordlow closed 9 months ago

nordlow commented 1 year ago

It's unfortunate that D decided to use the naming declaration instead of definition for grammar nodes such FuncDeclaration.

Because they aren't declarations they are in most cases definitions. The grammar should ideally distinguish between declarations and definitions as is done at, for instance, at

https://github.com/tree-sitter/tree-sitter-c/blob/master/grammar.js#L189 https://github.com/tree-sitter/tree-sitter-c/blob/master/grammar.js#L393

.

I presume the reason for this naming is that the D compiler, dmd and derivates ldc and gdc, chose the AST-node-namin ...Declaration instead of ...Definition.

Other tree-sitter grammars also use the same term definition at, for instance, https://github.com/tree-sitter/tree-sitter-python/blob/master/grammar.js#L385.

I believe this should be corrected just as your fork (reworked grammar) recently corrected func_declaration to function_declaration`.

Moreover, FuncDeclaration should be renamed to FunctionDeclarator. However, function_declarator is not part of the grammar.js. Shouldn't it?

gdamore commented 1 year ago

So a couple of points here:

  1. This work isn't a fork of another project. It was coded by hand using other projects for reference material.
  2. Yes, the use of Declaration stems from the terminology on the official website which hosts the official grammar (which unfortunately has numerous problems besides this).

Having said that, I agree that the terminology should probably change from declaration to definition for those things that are indeed definitions. I will review.

Thanks for pointing this out.

nordlow commented 1 year ago

Thank you.

Ok, not a fork, got it. Sorry for the incorrect choice of word. I've corrected my statement above.

gdamore commented 1 year ago

So I think the reason that D calls all these things declarations is that D has really two kinds of entities in source code (apart from comments and the like) -- statements and declarations. A definition isn't a statement, so D's grammar calls it a declaration.

But I completely see the point that this is a definition, not a declaration. In languages like C the distinction is more prevalent, as function prototypes are common in places like headers. In D source, the use of prototypes appears to be quite rare -- interfaces and abstract classes, along with function/delegate types, appear to be the only places we see it.

gdamore commented 1 year ago

Also, be advised that not every entity that appears as a node on the official grammar spec appears as a node in my grammar.js. There are various reasons for this -- some of them are just implementation ramifications (the official spec is a recursive descent grammar, whereas tree-sitter is GLR) and some of them reflect actual bugs in the official grammar, or possibly (I hope not!) in my grammar.

gdamore commented 1 year ago

Looking at my grammar in particular, it looks like there isn't a discrimination between function declarations and function definitions. That's because the definition for function_body allows both:

    function_body: ($) =>
      choice(
        seq(optional($._in_out_contract_expressions), "=>", $._expr, ";"),
        seq(repeat($._function_contract), optional($.do), $.block_statement),
        seq(repeat($._function_contract), ";"),
        seq(repeat($._function_contract), $._in_out_statement)
      ),

In particular, ";" is allowed as the meaty part of the body.

Is it important for you to be able distinguish these two cases? I can change the grammar to do this, but it will add some extra "expansion" to the grammar, so I don't want to do this unless there is value to be obtained.

nordlow commented 1 year ago

I just thought it was a good idea to have the grammar match corresponding rules in c and c++ tree sitter grammars. No requirement on my side.

gdamore commented 9 months ago

I'm closing this for now. It's not worth exploding the grammar.