Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Longident.parse is deprecated #127

Open
jamesjer opened this issue May 8, 2020 · 4 comments
Open

Longident.parse is deprecated #127

jamesjer opened this issue May 8, 2020 · 4 comments

Comments

@jamesjer
Copy link

jamesjer commented May 8, 2020

Fedora Rawhide currently has a prerelease of OCaml 4.11. Building ppxlib now fails like so:

File "src/gen/import.ml", line 14, characters 21-36:
14 |   let lident x = mk (Longident.parse x)
                          ^^^^^^^^^^^^^^^
Error (alert deprecated): Longident.parse
this function may misparse its input,
use "Parse.longident" or "Longident.unflatten"

What is the right thing to do? Should I disable the alert and leave the code as is? Would it be correct to replace (Longident.parse x) with (Parse.longident (Lexing.from_string x))?

@ghost
Copy link

ghost commented May 11, 2020

We should disable the alert in ppxlib. In the upcoming ppx project, we should stop using this function and also change the signature of this helper. The current method is too stringly typed.

@pitag-ha
Copy link
Member

pitag-ha commented Jun 1, 2021

I've just seen this. With the change to Astlib, on new compilers I have indeed replaced (Longident.parse x) with (Parse.longident (Lexing.from_string x)) now (see here). @jeremiedimino , what did you mean with "also change the signature of this helper"? Would you take a lexbuf as input rather than a string?

@ghost
Copy link

ghost commented Jun 1, 2021

IIRC, there is a function lident : string -> Longident.t that alloows one to write lident "Foo.bar" to produce a Lident (Ldot (Lident "Foo", "Bar")). That seems too magical. When we want to write a constant identifier in the code we should write the structured version: Lident (Ldot (Lident "Foo", "Bar")), or alternatively use a function that makes it clear it's doing parsing, for instance by seeing "parse" in the function name.

Put another way, I would expect a function lident : string -> Longident.t to be just let lident x = Lident x. I wouldn't expect it to create a lexing buffer and do some lexing and parsing.

@pitag-ha
Copy link
Member

pitag-ha commented Jun 8, 2021

Ok, I see. In Astlib that should be fine then since I've called that function Longident.parse. I'll leave this open though, since the confusion you're describing is still in Ppxlib.Ast_builder.

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

No branches or pull requests

2 participants