Chapel: chpldoc renders some types as AppendExpr

[ben-albrecht]

Below are instances of types being rendered as some variant of AppendExpr by chpldoc. I've attached the associated code below each instance.

atomicVar:

var atomicVar : if hasABASupport then _ddata(_ABAInternal(objType?)) else atomic uint(64);

buffer.append():

proc buffer.append(buf:buffer, part:buffer_range = buf.all()) throws {
  ...
}

map.keys (this has now been no-doc'd):

var keys: domain(keyType, parSafe=parSafe);

---

I believe this is a bug in chpldoc.

[ben-albrecht]

@lydia-duncan - here's the issue I mentioned earlier offline.

[immadisairaj]

@ben-albrecht @lydia-duncan I would like to work on this issue.
@ben-albrecht I would like to know the expected output for the above mentioned.
And, in map.keys, I did not encounter any AppendExpr

[lydia-duncan]

Sure, that'd be great! (though I should mention that code freeze is today, so we're unlikely to merge the result until after the branch is cut)

[ben-albrecht]

I would like to know the expected output for the above mentioned.

I am not sure actually. I just documented the problem in this issue. One way to approach this is to distill these render errors down into a common denominator. From there, we can discuss how that minimal reproducer should be rendered with chpldoc and then have those rules apply to the more complicated cases mentioned in this issue.

And, in map.keys, I did not encounter any AppendExpr

I believe this one has been pragma "no doc"'d so it no longer is shown.

[immadisairaj]

though I should mention that code freeze is today, so we're unlikely to merge the result until after the branch is cut

@lydia-duncan yeah, that is totally fine

One way to approach this is to distill these render errors down into a common denominator. From there, we can discuss how that minimal reproducer should be rendered with chpldoc and then have those rules apply to the more complicated cases mentioned in this issue.

@ben-albrecht Sure, I will try that.

I believe this one has been pragma "no doc"'d so it no longer is shown.

yes, it is. But still I'll try to check that too. So, I could get some better understanding

[immadisairaj]

While going through the code, I found the following in test files which has the same problem

type t = [1..10] int;

var x = [i in 1..10] i;

[lydia-duncan]

Good to know! I should point out that the place that generates these AppendExpr messages tends to number them (AppendExpr corresponds to the function generating the error message, the number is the placement in the function), so it is entirely possible they are separate cases.

[immadisairaj]

I would like to link the test files for those cases
type t = [1..10] int; : link
var x = [i in 1..10] i; : link

I would like to know if these to be done or not. else, we can remove those test files I guess.

var atomicVar : if hasABASupport then _ddata(_ABAInternal(objType?)) else atomic uint(64);

For the above code, the chpldoc currently is not able to support if expressions.
So, I am trying to add IfExpr support to the AstToText. Then it will be able to give the complete statement into documentation. Or should it be something else?

[lydia-duncan]

It looks like the two files you link and the atomicVar case land in at least the same AppendExpr failure mode, though the solutions for them may be separable. They are definitely still bugs regardless.

So, I am trying to add IfExpr support to the AstToText. Then it will be able to give the complete statement into documentation. Or should it be something else?

I think that's the right path. We have a separate AST node for CondStmt, which is the statement version. We just will want to be careful that adding it doesn't cause code to turn up in the documentation that we don't want

[immadisairaj]

The following also has the same issue of if expression

type RandomStream = if defaultRNG == RNG.PCG then PCGRandomStream
                                               else NPBRandomStream;

docs link

[immadisairaj]

I am yet to find the problem with the following three

var keys: domain(keyType, parSafe=parSafe);

```chapel
type t = [1..10] int;

```chapel
var x = [i in 1..10] i;

I will be opening a PR for the other three (if expression and sub call). So, we can update it if there are any conflicts with others.

[immadisairaj]

Which type of expressions does [1..10] fall into? I am finding it a bit hard to find.

[lydia-duncan]

I think it's a domain expression? I'm not as familiar with the handling of that in compilation - I believe they are replaced with a different strategy during the normalize pass (which shouldn't get run by chpldoc). Your best best is to use --log and look at the generated AST starting from post-parsing.

[immadisairaj]

@lydia-duncan Thanks for the help. But it didn't turn out to be any domain expression. From the log it says [ (199512 call chpl__ensureDomainExpr(199508 call chpl_build_bounded_range 1 10)) ] which means DomainExpr is from after [. So, I tried to keep checking again and found astTagAsString() method which gave me that it is a LoopExpr. Now for that to display in docs, we have to add LoopExpr too similar to IfExpr.

[ben-albrecht]

Which type of expressions does [1..10] fall into? I am finding it a bit hard to find.

I think this depends on the context, but in the case mentioned above, I think it would be considered the short-hand notation for a "forall expression", which ends up getting stored as an array when assigned to a variable.

var x = [i in 1..10] i; // x is an array with type: `[1..10] i.type`

Not sure if this will help for the implementation, but should help determine how this should be rendered in the chpldocumentation.

See forall section of spec for more info on this syntax.

[immadisairaj]

@ben-albrecht Thanks, I already traced back the type and got the same as you said which is forall loop.
I have another doubt.
I don't know why there is seperate var i prints in chpldoc when I have var x = [i in 1..10] i; statement. Is it because the compiler is considering i as a separate type?

[lydia-duncan]

I believe the compiler is outputting that it created i (but I don't think there's any reason that should go in the user's documentation if we can avoid it). Are you checking the docs with CHPL_DEVELOPER=true or the --devel flag? I thought we had avoided generating certain documentation for users versus developers but I may be misremembering

[immadisairaj]

I tried making CHPL_DEVELOPER false, but still it didn't work. I did not use any flags.
I tried printing my Chapel env, I don't see any CHPL_DEVELOPER variable.

[lydia-duncan]

Ah, okay. Can you paste a copy of the generated documentation you're getting? ~And is i getting output before your change or just turning up as a result of it?~ (Nevermind, dumb question)

[immadisairaj]

Ah, okay. Can you paste a copy of the generated documentation you're getting?

.. default-domain:: chpl

.. module:: foo

foo
===
**Usage**

.. code-block:: chapel

   use foo;

.. data:: var i

.. data:: var x = [i in 1..10] i

The code for the generated document is

var x = [i in 1..10] i;

[lydia-duncan]

Okay. Can you break on when the documentation is occurring for i (using gdb or lldb) and check to see if it has FLAG_COMPILER_GENERATED?

[immadisairaj]

@lydia-duncan I think that the compiler creates the variable i and while documenting the variable i is also being printed as this is also present.
I checked for the FLAG_COMPILER_GENERATED flag right before it is added into the rst.txt file, and the hasFlag(FLAG_COMPILER_GENERATED) gives out false.

[lydia-duncan]

I suppose technically having it in the forall expression does declare it, so I suppose it makes sense not to have it marked "compiler generated" (though I would be interested in hearing from @vasslitvinov who I believe is our expert on forall expressions). However, I don't think we should document it - its scope is the forall expression and no one should be able to reference it outside of that expression.

If Vass doesn't think it's appropriate to mark that variable as "compiler generated", then perhaps we should mark it with FLAG_NO_DOC?

[lydia-duncan]

(It may take Vass a while to respond, as a heads up)

[immadisairaj]

@lydia-duncan can you please review #15412. And Maybe we can solve the issue for
var x = [i in 1..10] i; in another PR?

[lydia-duncan]

I think that's reasonable, I'll take a look at the PR now :)

[lydia-duncan]

This issue was resolved by #15412. I'll open a separate issue for the stray iteration variable.

[lydia-duncan]

Opened #15578