Regression: comment on labeled argument formatted away

[cknitt]

The comment in

@@uncurried

let f = (
  // comment
  ~a,
) => a

is preserved when formatting with 10.1.3, but gets removed when formatting with master:

@@uncurried

let f = (~a) => a

[cristianoc]

Works on master.

[cknitt]

Confirmed, this is working again now.

[cknitt]

@cristianoc Actually the problem is still there, but it happens in uncurried mode only.

[cristianoc]

So that position is something that does not correspond to anything in the parse tree. There's going to be some workaround to represent a comment in that position, which does not like the transformation to uncurried function definition.

Something perhaps even more important, is that one cannot put doc comments in that position (syntax error). CC @zth who had been asking about this last thing.
Generically speaking, since there's no way to store doc comments on arguments, they need to be stored on the function. So e.g. (a,b) => ... has 2 functions in the parse tree, and that's where the doc comments would be stored.

As a separate thought, I was wondering whether normal comments should be treated exactly like doc comments. That would limit the positions comments can be places, but would imply the comments are in the AST so the outcome would be much more predictable.
While this is probably possible for /* ... */ comments, I'm not sure at all about line comments // ... which perhaps need a special treatment anyway.

[cristianoc]

Had a chat with Intelligentia A. about the topic. Turns out there's something one could do and has not been explored:

Context

This discussion revolves around the ReScript language, which uses a handwritten parser and is bound to the existing OCaml AST definitions for compatibility reasons. The challenge is to find a way to represent comments in the AST, particularly when it comes to pretty printing and doc comments, while preserving compatibility with other tools.

Proposed Solution: Extended OCaml AST

A proposed solution is to create a pure extension of the OCaml AST, where nodes only become bigger to store additional information, such as comments. This extended AST would be written to disk, maintaining compatibility with other tools.

Benefits

• Allows storing comment information directly within the AST.
• Enables improved comment handling during pretty printing.
• Maintains compatibility with existing tools that rely on the OCaml AST.

Potential Dangers

• Assumes that OCaml's marshaling mechanism is forward-compatible and can handle the extended AST.
• Assumes that existing tools can gracefully ignore any unknown data in the extended AST.
• Performance impact due to larger nodes.

Testing

The extended AST has been tested to ensure that the OCaml serialization and deserialization mechanisms can handle the extended AST gracefully, and the tools can ignore the extra information.

Conclusion

Given the successful test results, the extended OCaml AST approach seems to be a viable solution for representing comments in the ReScript language while preserving compatibility with other tools. Thorough testing with various ReScript programs and different tools and libraries is crucial to ensure the robustness and compatibility of this solution. Engaging with the ReScript and OCaml communities can provide additional insights and feedback to further refine the approach.

[zth]

This seems to be a parser issue only, because attributes attach fine to the arguments (using ns.doc, with res.doc it's autoformated to a docstring, which right now is a syntax error):

let someFunc = (
  @ns.doc("Fine arg.") ~arg1: bool,
  @ns.doc("Fine arg 2.") ~arg2: option<string>=?,
  @ns.doc("Fine arg 3.") arg3: string,
) => {
  ignore(arg1)
  ignore(arg2)
  ignore(arg3)
}

Haven't checked where the attributes end up though, but I assume it must be on the function. And haven't checked whether they "make it" to the typed tree. But they should if they end up attached to the function.

I'm very interested in supporting this use case. Both for the WIP doc extraction, and for leveraging it in the editor for showing documentation.

[cristianoc]

[
  structure_item
    ...
      <def>
        pattern
          Ppat_var "someFunc"
        expression
          Pexp_fun
          Labelled "arg1"
          None
          pattern
            attribute "res.namedArgLoc"
              []
            attribute "ns.doc"
              [
                ...
                expression
                  Pexp_constant PConst_string ("Fine arg.", Some "*j")
              ]
            Ppat_constraint
            pattern
              Ppat_var "arg1"
            core_type
              Ptyp_constr "bool"
              []
          expression
            Pexp_fun
            Optional "arg2"
            None
            pattern
              attribute "res.namedArgLoc"
                []
              attribute "ns.doc"
                [
                  ...
                  expression
                    Pexp_constant PConst_string ("Fine arg 2.", Some "*j")
                ]
              Ppat_constraint
              pattern
                Ppat_var "arg2"
              core_type
                Ptyp_constr "option"
                [
                  core_type
                    Ptyp_constr "string"
                    []
                ]
            expression
              Pexp_fun
              Nolabel
              None
              pattern
                attribute "ns.doc"
                  [
                    ...
                    expression
                      Pexp_constant PConst_string ("Fine arg 3.", Some "*j")
                  ]
                Ppat_constraint
                pattern
                  Ppat_var "arg3"
                core_type
                  Ptyp_constr "string"
                  []
              expression
                ...
]

[cristianoc]

[cristianoc]

So yes ^ it's on the function.

[zth]

Can you see whether it ends up in the typed tree?

[cristianoc]

Can you see whether it ends up in the typed tree?

I know it does. But I can double check.

[cristianoc]

It is actually not true. I know it does for places where there's a natural place to store the attribute.
It is not the case for the function. Or perhaps the printer of typed expressions does not show it.

structure_item
  └─ Tstr_value
      └─ <def>
          └─ expression
              └─ Texp_function (arg1)
                  └─ <case>
                      └─ expression
                          └─ Texp_function (arg2)
                              └─ <case>
                                  └─ expression
                                      └─ Texp_function (arg3)
                                          └─ <case>
                                              └─ expression
                                                  ├─ Texp_apply (ignore, arg1)
                                                  ├─ Texp_apply (ignore, arg2)
                                                  └─ Texp_apply (ignore, arg3)

[cristianoc]

This is the parse tree:

            expression 
              Pexp_fun
              Nolabel
              None
              pattern 
                attribute  "ns.doc"
                  [
                    structure_item 
                      Pstr_eval
                      expression 
                        Pexp_constant PConst_string ("Fine arg 3.",Some "*j")
                  ]
                Ppat_constraint
                pattern 
                  Ppat_var "arg3" 
                core_type 
                  Ptyp_constr "string" 
                  []

This is the typed tree (printer modified to show the number of attributes):

                   Texp_function
                      arg3/1005                      Nolabel
                      [
                        <case>
                          pattern  attributes:0
                            Tpat_constraint
                            core_type 
                              Ttyp_constr "string/13"
                              []
                            pattern  attributes:0
                              Tpat_alias "arg3/1005"
                              pattern  attributes:0
                                Tpat_any
                          expression 
                            attribute "res.braces"
                              []

So it looks like the attributes are indeed lost.

[cristianoc]

So one needs to:

1. adapt the parser
2. adapt the type checker to preserve the attributes
3. pick them up from the pattern in the editor extension

[cristianoc]

Perhaps this is a good opportunity to test the ast extension idea where:
1 extend the function AST to allow attributes for arguments
2 adapt the type checker (and typed ast) to propagate them
3 check in the editor extension if we're working with the extended (typed) AST (for backwards compatibility), and use the info from there

[cristianoc]

Can you see whether it ends up in the typed tree?

yes it goes nowhere

[cristianoc]

Here's an example PR that extends the parse tree with attributes on function definitions: #6155

After doing that, it occurred to me that this was misguided. In this specific example, putting the attribute on the pattern is the correct thing to do. Kind of forgot that x => x+1 really is pattern matching on the catch-all pattern named x, and it does not have to be that way. So adding the attribute does not always makes sense as x could be something else. In any case, whatever is in x position is a pattern. And patterns already store attributes. So this was just the wrong example.

However, the PR still illustrates how to extend the parse tree, so I've out it up anyway.

The real place where the attribute is missing, is in the function type declaration, not in the function declaration, and one can look into that separately.

[cristianoc]

On further investigation, the typed tree for t1 => t2 is a core_type which does have attributes.
Only, this example:

let foo = (@res.doc("ddd") ~x) => x+1

does not have any explicit types, so there's nothing to look at in the typed tree.
Instead, the example could e.g. be written as:

let foo: (@res.doc("ddd") ~x: int) => int = (~x) => x + 1

This latter example has the doc comment in the typed tree:

  structure_item 
    Tstr_value Nonrec
    [
      <def>
        pattern 
          Tpat_constraint
          core_type 
            attribute "res.doc"
              [
                structure_item 
                  Pstr_eval
                  expression 
                    Pexp_constant PConst_string ("ddd",Some "*j")
              ]
            Ttyp_arrow
            Labelled "x"
...

CC @zth what do you think?

[zth]

This essentially means that you'd need an explicit type definition for the function to be able to use docstrings for args. You have that in these cases:

• Using interface files
• Using module constraints (or w/e we call them, module X: { let f: unit => unit })
• Bindings, because externals are always type definitions
• Hand written "ad hoc" type for a function, like a callback

The cases where it won't work is for ad hoc functions that you rely on inference for. But those shouldn't be the main case needing docstrings anyway, compared to the cases listed above.

Sounds like a perfectly acceptable situation to me. @cristianoc fixing so that one can use the dedicated docstrings syntax should be easy, right?

[cristianoc]

Fixing the parser should be easy.
The rest, is the editor extension needing to pick up the info.

[zth]

@cristianoc you do the parser, I do the extension?

[cristianoc]

@cristianoc you do the parser, I do the extension?

Parser: #6161

[cristianoc]

The original issue is fixed here: #6169