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

[BUG] caml_float_of_string's handling of whitespace doesn't match Stdlib #1518

Open
tpetr opened this issue Oct 17, 2023 · 1 comment
Open
Labels

Comments

@tpetr
Copy link

tpetr commented Oct 17, 2023

Describe the bug
jsoo's implementation of caml_float_of_string doesn't match Stdlib's handling of whitespace: jsoo allows trailing whitespace while regular ocaml does not

jsoo toplevel:

        OCaml version 5.0.0
     Compiled with Js_of_ocaml version 5.4.0+e2e1f3d
float_of_string "3.14";;
- : float = 3.14
float_of_string " 3.14";;
- : float = 3.14
float_of_string "3. 14";;
Exception: Failure "float_of_string".
float_of_string "3.14 ";;
- : float = 3.14

ocaml toplevel:

OCaml version 4.14.0
Enter #help;; for help.

Findlib has been successfully loaded. Additional directives:
  #require "package";;      to load a package
  #list;;                   to list the available packages
  #camlp4o;;                to load camlp4 (standard syntax)
  #camlp4r;;                to load camlp4 (revised syntax)
  #predicates "p,q,...";;   to set these predicates
  Topfind.reset();;         to force that packages will be reloaded
  #thread;;                 to enable threads

# float_of_string "3.14";;
- : float = 3.14
# float_of_string " 3.14";;
- : float = 3.14
# float_of_string "3. 14";;
Exception: Failure "float_of_string".
# float_of_string "3.14 ";;
Exception: Failure "float_of_string".

(tl;dr float_of_string "3.14 " fails in regular ocaml but not jsoo)

Expected behavior
Trailing whitespace passed to float_of_string would raise an exception.

Versions
ocamlc 4.14
jsoo 5.4.0

(NOTE: this is an extremely low-priority bug, but wanted to flag it because we stumbled across it in semgrep.js: https://github.com/returntocorp/semgrep/pull/8902/files#diff-e2f964d2fdf546821a6fcd6c642efc8e962a8972a1731b8196d36ef2e0aa621dR44-R47)

@tpetr tpetr added the bug label Oct 17, 2023
tpetr added a commit to semgrep/semgrep that referenced this issue Oct 17, 2023
This PR does a bunch of things:
- Semgrep.js parsers:
  - Factors out the test harness plumbing into `js/shared/parser.js`
- Updates parsers to use official pattern / parsing test fixtures, which
means more test coverage for jsoo
- Fixed a few parsers that didn't match their Parse_target2.ml /
Parse_pattern2.ml counterparts
- fixed issues with utf-8 in libpcre.js
- jsoo's caml_float_of_string doesn't handle whitespace the same way as
Stdlib (issue: ocsigen/js_of_ocaml#1518)
- testing:
- added `js/tests` which runs the entire Semgrep test suite against the
semgrep.js artifacts
- (with some tests currently disabled for various reasons, see:
https://github.com/returntocorp/semgrep/pull/8902/files#diff-dd182dfc008e74db2491c9b572d2f1885943808bf40dd7f2d384a097c6f86a96R26-R39)
- due to how semgrep.js parsers work, had to make jsonnet parser
initialization lazy (See:
https://github.com/returntocorp/semgrep/pull/8902/files#diff-2ca9fa192a8e7f7613f16ab4969198be90b75bd5fe38ed3f79a1d23fb59f1e59R57-R59)
- Updated Entropy.ml to explicitly use Int64 so that it still works on
32-bit systems (namely: browsers)

(^ @tpetr wrote this -- @ajbt200128 , feel free to update)

test plan:
- checks pass

---------

Co-authored-by: Tom Petr <tom@r2c.dev>
@hhugo
Copy link
Member

hhugo commented Nov 3, 2023

Would you be able to contribute a PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants