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.

Sign up for GitHub

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

Jump to bottom

PPX: reconsider disabling warning 20 in all functions #1165

Open
jchavarri opened this issue Sep 3, 2024 · 4 comments · May be fixed by #1166
Open

PPX: reconsider disabling warning 20 in all functions #1165

jchavarri opened this issue Sep 3, 2024 · 4 comments · May be fixed by #1166

Comments

@jchavarri
Copy link
Member

In #915, the silencing of warning 20 was moved to all function applications (regardless if the function is defined with mel.raw or not).

I understand it's impossible to detect this kind of situation at PPX time. But I wonder if the silencing should be removed from the PPX, and instead document in mel.raw docs that these functions need to be explicitly annotated? E.g. if the ppx was modified to not add the attribute, this test would trigger the following results:

Usage of mel.raw can trigger warning 20 ([ignored-extra-argument])

  $ . ./setup.sh
  $ cat > dune-project <<EOF
  > (lang dune 3.8)
  > (using melange 0.1)
  > EOF
  $ cat > dune << EOF
  > (melange.emit
  >  (target out)
  >  (emit_stdlib false)
  >  (preprocess (pps melange.ppx)))
  > EOF

  $ cat > foo.ml <<EOF
  > let addOne = [%mel.raw {|
  >   function (a) {
  >     return a + 1;
  >   }
  > |}]
  > let x = addOne 2
  > EOF

  $ dune build @melange
  File "foo.ml", line 6, characters 15-16:
  6 | let x = addOne 2
                     ^
  Error (warning 20 [ignored-extra-argument]): this argument will not be used by the function.
  [1]

Annotating the function (which should always be the case, for safety) makes the
warning go away

  $ cat > foo.ml <<EOF
  > let addOne: int -> int = [%mel.raw {|
  >   function (a) {
  >     return a + 1;
  >   }
  > |}]
  > let x = addOne 2
  > EOF

  $ dune build @melange
@jchavarri
Copy link
Member Author

Another alternative to reconsider to force users to add the types of these unsafe JS snippets is to define them as externals, e.g.:

external addOne: int -> int = {raw_js|
  function (a) {
    return a + 1;
  }
|raw_js}
let x = addOne 2

@jchavarri jchavarri linked a pull request Sep 3, 2024 that will close this issue
Open
@davesnx
Copy link
Member

davesnx commented Sep 3, 2024 •
edited
Loading

melange uses flowparser to know the arity of the function, why do we make the user annotate the function in the first place?

@jchavarri
Copy link
Member Author

Because arity only is not enough to get the type of the function. And in JavaScript it's even worse, e.g. consider function (a,b) { return a + b }. Is the type (int -> int -> int[@u]) or (string -> string -> string[@u])?

@jchavarri
Copy link
Member Author

Oh, I guess you meant why the compiler can't give addOne a type 'a -> 'a rather than just 'a when there's no annotation, to avoid the warning 20s later on 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

PPX: don't silence warning 20 in fnapply melange-re/melange
2 participants
@jchavarri @davesnx