Skip to content

Conversation

@Octachron
Copy link
Member

This PR fixes #13603 by splitting the longident syntactic category in three inside the implementation of Pparsetree:

  • Contructor-like longident for which true and false are uppercase identifiers
  • Type-like longident for which mod is not an operator but the lowercase identifier written \#mod in the source.
  • Regular longident for which mod is an operator (which should be printed (mod)) and true and false are lowercase identifiers introduced with the raw identifier syntax #true and #false.

Beyond the specific bug discovered in #13603 this PR also fixes along the way:

  • the printing of true|false|[]|() constructors in presence of arguments
  • the printing of module type \#let.

Fix ocaml#13603 by not adding parentheses around (mod) when it is neither a
pattern nor an expression.
type longindent_kind =
| Constr (* [true] and [false] *)
| Type (* [mod] *)
| Other
Copy link
Member

Choose a reason for hiding this comment

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

I feel that I understand almost everything in this PR, except for this type declaration and its documentation.

  • I don't know what the comment ("Classify ...") means.
  • The documentation is specifically talking about raw identifiers, but I think that the meaning of this "kind" is independent from raw identifiers. (It's some information that we need to track because of raw identifier printing needs, but which is independent from it.)
  • My reverse-engineering guess is that this is pretty-printing context information, that carries information on the grammatical context in which the printed value will be used. Another way to view it is that we are specifying the grammatical category of what is being printed. (For a notion of grammar that is finer-grained than just "longident", obviously.) It seems to be related to (a coarser-grained version of) the type Out_type.namespace.

I think that if the documentation of this type (and maybe the type name?) was changed to better reflect my reverse-engineered guess, then the PR would look very nice to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is too distilled comments on the fact that Longident is a too coarse grammatical category to allow unparsing: the grammar rules for constructor, module, type and value longidents are different. Before the introduction of raw identifiers, we could recover just enough information to print them correctly by recognizing special case in each category (true and false for constructors, operators for values).

Raw identifiers makes this context-less reconstruction impossible by making it possible for those special case to appear escaped in other kind of longidents. For instance mod might be either an operator name in a value longident, or a raw identifier in a type longident. Similarly, true might be a normal constructor or a raw identifier in a value longident.

Thus this PR uses contextual information to recover the information about which form of longidents we are printing and how should we print them according to their specific grammar.

I will amend the comment to be more explicit (next week).

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

The new comment went above and beyond what I expected. Thanks! Approved.

@gasche gasche merged commit 75459af into ocaml:trunk Nov 18, 2024
Octachron pushed a commit that referenced this pull request Nov 19, 2024
-dsource: \#mod is not an operator in type context
(cherry picked from commit 75459af)
@Octachron
Copy link
Member Author

Cherry-picked on 5.3 as 8237eaa .

NathanReb pushed a commit to NathanReb/ppxlib that referenced this pull request Oct 8, 2025
This imports upstream patches to pprintast that were done post
5.2 and improves pprintast's handling of raw identifiers.
Hopefully that should fix the issue that fstar folks ran into
with the 0.36.2 release.

Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
NathanReb pushed a commit to NathanReb/ppxlib that referenced this pull request Oct 8, 2025
This imports upstream patches to pprintast that were done post
5.2 and improves pprintast's handling of raw identifiers.
Hopefully that should fix the issue that fstar folks ran into
with the 0.36.2 release.

Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
NathanReb pushed a commit to NathanReb/ppxlib that referenced this pull request Oct 8, 2025
This imports upstream patches to pprintast that were done post
5.2 and improves pprintast's handling of raw identifiers.
Hopefully that should fix the issue that fstar folks ran into
with the 0.36.2 release.

Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
NathanReb pushed a commit to NathanReb/ppxlib that referenced this pull request Oct 8, 2025
This imports upstream patches to pprintast that were done post
5.2 and improves pprintast's handling of raw identifiers.
Hopefully that should fix the issue that fstar folks ran into
with the 0.36.2 release.

Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
NathanReb pushed a commit to NathanReb/ppxlib that referenced this pull request Oct 8, 2025
This imports upstream patches to pprintast that were done post
5.2 and improves pprintast's handling of raw identifiers.
Hopefully that should fix the issue that fstar folks ran into
with the 0.36.2 release.

Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
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.

Bug in source printer with raw identifiers

2 participants