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

Issue 41: Signed-off-by: Jadesola Bello <[email protected]> #44

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fikayo-sanni
Copy link

Hello @panglesd

I am an outreachy applicant who is just getting started in the OCaml ecosystem

This is a attempt toward fixing issue here

I however have the following questions

  1. Is there any good formatter for OCaml on VS Code?
  2. What is the difference between pexp_extension and ppat_extension?
  3. How do I independently verify that the ppx works after making an edit to the codebase

@Burnleydev1
Copy link

  1. ocaml platform does the trick.
  2. In OCaml, "pexp_extension" and "ppat_extension" are both used to represent syntax extensions. The main difference between them is that "pexp_extension" is used to represent syntax extensions in expressions, while "ppat_extension" is used to represent syntax extensions in patterns. In other words, "pexp_extension" is used when you want to extend the syntax of an expression, and "ppat_extension" is used when you want to extend the syntax of a pattern.

@Burnleydev1
Copy link

Burnleydev1 commented Mar 18, 2023

  1. Once you clone this repo, install the dependencies if needed (using opam install <package>), and run dune build, before making modifications to the code.
    from there the errors you're getting will mostly be due to the changes you're making

@fikayo-sanni
Copy link
Author

Thanks @Burnleydev1 . I have been able to install Ocaml platform to my vs code, alongside OCaml indentation
I will run the code locally and fix the issues that come up.
Thanks for your help

@Burnleydev1
Copy link

Burnleydev1 commented Mar 19, 2023

Hello @fikkyJ, For the first error, I think you are missing an s after the expression "invalid 'inline-test' cookie (%s), expected one of: drop, drop_with_deadcode", so it should be "invalid 'inline-test' cookie (%s), expected one of: drop, drop_with_deadcode"s rather, that should solve the error of Error (warning 5): this function application is partial, maybe some arguments are missing.

@panglesd
Copy link

Hello @fikkyJ!

The error you were seeing was because the %s makes Location.error_extensionf ~loc:<...> "... %s ..." be a function string -> extension, where pexp_extension expects it to be a value of type extension.
Adding the s, as @Burnleydev1 suggested, would solve this, but there would still be errors: pexp_extension would end up creating an extension node, in a function which has return type unit!

As a start, I think you should focus on the errors in validate_extension_point_exn, and try to fix the example about multiple tags in #41 (comment). Collect the errors in validate_extension_point, and embed them at the call sites of this function.

@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Mar 21, 2023
@meghaaa30
Copy link

Hello @fikkyJ!

The error you were seeing was because the %s makes Location.error_extensionf ~loc:<...> "... %s ..." be a function string -> extension, where pexp_extension expects it to be a value of type extension. Adding the s, as @Burnleydev1 suggested, would solve this, but there would still be errors: pexp_extension would end up creating an extension node, in a function which has return type unit!

As a start, I think you should focus on the errors in validate_extension_point_exn, and try to fix the example about multiple tags in #41 (comment). Collect the errors in validate_extension_point, and embed them at the call sites of this function.

to avoid creating an extension node, can we change the return type from unit to expression or remove the pexp_extension function call altogether?

@fikayo-sanni
Copy link
Author

Hello @Burnleydev1 @panglesd

I have made the following changes to the PR

The following changes have been effected

  1. validate_extension_point_exn now returns a string List of errors thrown everytime the function is called

  2. A new function throw_exception_list has been created to throw the exceptions arising inside the Ast_builder

  3. I initiated the new function in the expand_test function; and I am passing the list of exceptions into the err variable, and then trying to add them to the ast via the ast_builder

I however now get an error from the ast_builder stating that

Error: This expression has type string but an expression was expected of type
         ('a, Format.formatter, unit, Ppxlib__Import.extension) format4 =
           ('a, Format.formatter, unit, unit, unit, Ppxlib__Import.extension)
           format6

@panglesd
Copy link

Hello @fikkyJ !

You are making good progress!

Your modification to the validate_extension_point_exn function is good! (The style is not the one of a functional language, but it already does (almost) what is expected! You should avoid using references. If you want to improve on this (I could be done later), have a look at the List.filter_map function!)

Instead of returning a list of strings,validate_extension_point_exn (this name will need to be changed) should return a list of located errors. This way, it would embed not only the text of the error, but also the location.

Your handling of the collected list should also change! The idea is exactly not to raise, but several parts in your code suggest this part is not clear yet.
The idea is, in case of error (that is, when the returned list is not empty) is to still return an AST ; but return one which contain a special kind of node (an error extension node), which will be recognized as an error by the compiler (and merlin, the editor helper). For instance, let _ = [%ocaml.error "an example of an error embedded in the AST"] is a perfectly valid AST, but it embeds an error. You could write this in an OCaml program, and see how the error is reported.
If this is not yet clear, you can:

  • re-read the original issue and the resources relative to embedding errors
  • have a look at the examples in the ppxlib repo, with how errors are embedded.

About the error you are seeing, it is because in OCaml there is a distinction between format string (which are the one were %s, %d, ... are interpreted) and normal string. When OCaml sees a constant string (not a variable), it is able to decide if it is a normal string or a format string. In your case, the err function has already been typed to a normal string, so you cannot use it as a format string. You should use the format string "%s", which expects a normal string to replace the %s.
However, this error only hides other errors, due to your misunderstanding in raising vs embedding errors!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants