Add tests for parsing longident's with . #111

rgrinberg · 2019-12-22T11:15:07Z

There are some identifiers that have the . character. We test cases with +.
and *.

It seems like the +. isn't handled correctly when there's no module prefixing.

rgrinberg · 2019-12-22T15:26:44Z

@gasche, i'm a bit curious, why is the implementation of Longident.parse in compiler-libs not complete? I'm being bit by a bug in it in the context of merlin as well.

gasche · 2019-12-22T17:15:53Z

parse is a misnomer for this function that is used in very restricted ways inside the compiler codebase, only to parse paths of the form Foo.Bar.baz. It was never designed for external usage.

The robust code to parse longidents is in the parser, but it separates in many smaller categories (mod_longident similarly does not allow module applications, mod_ext_longident does, and both of those only parse module identifiers (the last component is uppercased) and get turned into various non-module-identifiers (for values, types, etc.)) that are finer grained than the type Longident.t represents. We could propose to expose this parser logic, but:

I don't know what would be a good API for it. (We could expose the various kind of longidents, one for each namespace: modules, types and module types, values, classes and class types.)
We cannot expose this from Longident.parse, because it would create a cyclic dependency between the parser and Longident. So users would have to access Parse.*_longident instead, and build a lexing buffer themselves.

Finally, we could also try to use a hand-written parser for Longident -- for example the one I proposed in #98 (comment) -- but it seems weird to duplicate logic that we already have someplace else, with inferior technology, for something we don't need inside the compiler codebase.

NathanReb

There is something weird about the way the exceptions are reported in the tests.

It seems that 4.08 reports them differently from 4.07 in the CI but when I try to reproduce that locally, I get the same formatting (no parens, ends with a .) with both 4.07 and 4.08.

Besides that it looks good!

@gasche I'm happy to import your suggested implementation in ppxlib when I get some time to work on that. In the meantime, if you wish to open a PR and fix the tiny bugs it contains so that it passes the existing tests that would be wonderful.

There are some identifiers that have the `.` character. We test cases with +. and *. Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>

panglesd

About the "add tests" part: Let's merge, it's good to have more tests!

About the "fix a longident bug about unparenthesized operators": that looks good and fixing a bug. I don't know enough the possible syntax of longidents to be confident there aren't other bugs though!

src/longident.ml

test/base/test.ml

src/longident.ml

Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>

@rgrinberg

CHANGES: - Fix `Longident.parse` so it properly handles unparenthesized dotted operators such as `+.` or `*.`. (ocaml-ppx/ppxlib#111, @rgrinberg, @NathanReb) - raising an exception does no longer cancel the whole context free phase(ocaml-ppx/ppxlib#453, @Burnleydev1) - Sort embedded errors that are appended to the AST by location so the compiler reports the one closer to the beginning of the file first. (ocaml-ppx/ppxlib#463, @NathanReb) - Update `Attribute.get` to ignore `loc_ghost`. (ocaml-ppx/ppxlib#460, @ceastlund) - Add API to manipulate attributes that are used as flags (ocaml-ppx/ppxlib#408, @dianaoigo) - Update changelog to use ISO 8061 date format: YYYY-MM-DD. (ocaml-ppx/ppxlib#445, @ceastlund) - Replace `Caml` with `Stdlib`. (ocaml-ppx/ppxlib#427, @ceastlund) - When a transformation raises, the last valid AST is used as input to the upcoming transformations. All such errors are collected and appended as extension nodes to the final AST (ocaml-ppx/ppxlib#447, @Burnleydev1) - Fix a small mistake in the man pages: Embededding errors is done by default with `-as-pp`, not with `-dump-ast` (ocaml-ppx/ppxlib#464, @pitag-ha) - Set appropriate binary mode when writing to `stdout` especially for Windows compatibility. (ocaml-ppx/ppxlib#466, @jonahbeckford)

@NathanReb

CHANGES: - Add an optional `embed_errors` argument to `Context_free.map_top_down` that controls how to deal with exceptions thrown by context-free rules. (ocaml-ppx/ppxlib#468, @NathanReb) - Fix `Longident.parse` so it properly handles unparenthesized dotted operators such as `+.` or `*.`. (ocaml-ppx/ppxlib#111, @rgrinberg, @NathanReb) - raising an exception does no longer cancel the whole context free phase(ocaml-ppx/ppxlib#453, @Burnleydev1) - Sort embedded errors that are appended to the AST by location so the compiler reports the one closer to the beginning of the file first. (ocaml-ppx/ppxlib#463, @NathanReb) - Update `Attribute.get` to ignore `loc_ghost`. (ocaml-ppx/ppxlib#460, @ceastlund) - Add API to manipulate attributes that are used as flags (ocaml-ppx/ppxlib#408, @dianaoigo) - Update changelog to use ISO 8061 date format: YYYY-MM-DD. (ocaml-ppx/ppxlib#445, @ceastlund) - Replace `Caml` with `Stdlib`. (ocaml-ppx/ppxlib#427, @ceastlund) - When a transformation raises, the last valid AST is used as input to the upcoming transformations. All such errors are collected and appended as extension nodes to the final AST (ocaml-ppx/ppxlib#447, @Burnleydev1) - Fix a small mistake in the man pages: Embededding errors is done by default with `-as-pp`, not with `-dump-ast` (ocaml-ppx/ppxlib#464, @pitag-ha) - Set appropriate binary mode when writing to `stdout` especially for Windows compatibility. (ocaml-ppx/ppxlib#466, @jonahbeckford)

@NathanReb

CHANGES: - Add an optional `embed_errors` argument to `Context_free.map_top_down` that controls how to deal with exceptions thrown by context-free rules. (ocaml-ppx/ppxlib#468, @NathanReb) - Fix `Longident.parse` so it properly handles unparenthesized dotted operators such as `+.` or `*.`. (ocaml-ppx/ppxlib#111, @rgrinberg, @NathanReb) - raising an exception does no longer cancel the whole context free phase(ocaml-ppx/ppxlib#453, @Burnleydev1) - Sort embedded errors that are appended to the AST by location so the compiler reports the one closer to the beginning of the file first. (ocaml-ppx/ppxlib#463, @NathanReb) - Update `Attribute.get` to ignore `loc_ghost`. (ocaml-ppx/ppxlib#460, @ceastlund) - Add API to manipulate attributes that are used as flags (ocaml-ppx/ppxlib#408, @dianaoigo) - Update changelog to use ISO 8061 date format: YYYY-MM-DD. (ocaml-ppx/ppxlib#445, @ceastlund) - Replace `Caml` with `Stdlib`. (ocaml-ppx/ppxlib#427, @ceastlund) - When a transformation raises, the last valid AST is used as input to the upcoming transformations. All such errors are collected and appended as extension nodes to the final AST (ocaml-ppx/ppxlib#447, @Burnleydev1) - Fix a small mistake in the man pages: Embededding errors is done by default with `-as-pp`, not with `-dump-ast` (ocaml-ppx/ppxlib#464, @pitag-ha) - Set appropriate binary mode when writing to `stdout` especially for Windows compatibility. (ocaml-ppx/ppxlib#466, @jonahbeckford)

rgrinberg requested a review from a user December 22, 2019 11:15

rgrinberg requested a review from xclerc as a code owner December 22, 2019 11:15

rgrinberg requested a review from NathanReb December 22, 2019 11:15

gasche mentioned this pull request Dec 22, 2019

merlin-locate does not work on +. ocaml/merlin#949

Closed

NathanReb reviewed Dec 26, 2019

View reviewed changes

rgrinberg force-pushed the longident-bug branch from 4512f51 to 3bd7659 Compare January 7, 2020 12:59

panglesd requested review from ceastlund, panglesd and pitag-ha as code owners September 26, 2023 10:01

Add tests for parsing longident's with .

3abce85

There are some identifiers that have the `.` character. We test cases with +. and *. Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

NathanReb force-pushed the longident-bug branch from 3bd7659 to 3abce85 Compare January 31, 2024 08:56

NathanReb added 2 commits January 31, 2024 10:12

Fix exception raising test cases

88b3c94

Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>

Add extra test case for operator with parens

3ee3d52

Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>

panglesd approved these changes Jan 31, 2024

View reviewed changes

src/longident.ml Outdated Show resolved Hide resolved

test/base/test.ml Outdated Show resolved Hide resolved

NathanReb force-pushed the longident-bug branch 2 times, most recently from a2013ef to 4389da7 Compare January 31, 2024 10:50

panglesd reviewed Jan 31, 2024

View reviewed changes

src/longident.ml Outdated Show resolved Hide resolved

Fix Longident parsing of unparenthesized dotted operators

fa75e23

Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>

NathanReb force-pushed the longident-bug branch from 4389da7 to fa75e23 Compare January 31, 2024 12:57

NathanReb merged commit 9b55562 into ocaml-ppx:main Jan 31, 2024
5 checks passed

NathanReb mentioned this pull request Feb 1, 2024

[new release] ppxlib (0.32.0) ocaml/opam-repository#25152

Closed

NathanReb mentioned this pull request Feb 5, 2024

[new release] ppxlib (0.32.0) ocaml/opam-repository#25179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for parsing longident's with . #111

Add tests for parsing longident's with . #111

rgrinberg commented Dec 22, 2019

rgrinberg commented Dec 22, 2019

gasche commented Dec 22, 2019 •

edited

Loading

NathanReb left a comment

panglesd left a comment

Add tests for parsing longident's with . #111

Add tests for parsing longident's with . #111

Conversation

rgrinberg commented Dec 22, 2019

rgrinberg commented Dec 22, 2019

gasche commented Dec 22, 2019 • edited Loading

NathanReb left a comment

Choose a reason for hiding this comment

panglesd left a comment

Choose a reason for hiding this comment

gasche commented Dec 22, 2019 •

edited

Loading