Reason: Error compiling infer with reason 1.7.4 (opam)

[jvillard]

I upgraded my opam packages and now infer does not compile anymore:

$ git clone --depth 1 https://github.com/facebook/infer
$ cd infer
$ ./build-infer.sh java
[...]
+ ocamlfind ocamlc -c -g -short-paths -safe-string -principal -strict-formats -strict-sequence -w +3+5+6+8+10+11+12+18+19+20+23+26+29+27+33+34+35+37+38+39+50-4-9-32-40-41-42-45-48 -bin-annot -thread -package sawja -package ptrees -package javalib -package zip -package yojson -package xmlm -package unix -package str -package oUnit -package ctypes.foreign -package core -package atdgen -package ppx_compare -pp '-g -short-paths -safe-string -principal -strict-formats -strict-sequence -w +3+5+6+8+10+11+12+18+19+20+23+26+29+27+33+34+35+37+38+39+50-4-9-32-40-41-42-45-48 -bin-annot -thread -package sawja -package ptrees -package javalib -package zip -package yojson -package xmlm -package unix -package str -package oUnit -package ctypes.foreign -package core -package atdgen -package ppx_compare' -I IR -I backend -I checkers -I harness -I bufferoverrun -I base -I eradicate -I integration -I java -I quandary -I facebook -I tp/fts -I facebook/scripts -pp 'refmt --print binary' -o IR/StructTyp.cmi -intf-suffix .rei -intf IR/StructTyp.rei
File "IR/StructTyp.rei", line 22, characters 17-237:
Uninterpreted extension 'merlin.syntax-error'.

The relevant line of code is a blank line in between type declarations: https://github.com/facebook/infer/blob/574d640af03871058e37aa77d7cf9f8a81fec591/infer/src/IR/StructTyp.re#L22

EDIT: derp, .rei, not .re, so the relevant line is a record type declaration: https://github.com/facebook/infer/blob/574d640af03871058e37aa77d7cf9f8a81fec591/infer/src/IR/StructTyp.rei#L22

[jvillard]

Others are reporting this issue on the infer repo too: https://github.com/facebook/infer/issues/577

[chenglou]

@SanderSpies this is fixed by https://github.com/facebook/reason/pull/1049 right?

[jordwalke]

That's what I believe. We may just need to push a new opam version with the fix.

[jvillard]

Thanks! Does that mean that there is a genuine compilation error in infer with the latest reason?

[jordwalke]

After closer inspection, I think this might be another issue. Why is the parse failing at all?

[SanderSpies]

I'll take a look. Probably my bad.

[jvillard]

Pinning reason to latest master, I get:

File "IR/StructTyp.rei", line 22, characters 17-237:
Record type is not allowed
File "IR/StructTyp.rei", line 1:
Error: Error while running external preprocessor
Command line: refmt --print binary 'IR/StructTyp.rei' > /tmp/ocamlpp48ebdd

[jvillard]

Relevant source code in StructType.rei:

type t = private {
  fields: fields, /** non-static fields */
  statics: fields, /** static fields */
  supers: list Typename.t, /** supers */
  methods: list Procname.t, /** methods defined */
  annots: Annot.Item.t /** annotations */
};

[jordwalke]

Oh, I believe the error was poorly printed, and then master fixed the printing of the error, but the error is legitimate. I believe this is because we've replaced the private keyword with pri, as part of the improvements to object syntax. Is this correct (Sanders?)

let myObject = {
  pub x () => x;
  pri internalMethod () => 0;
};

[jvillard]

If I remove private then that file goes through.

[SanderSpies]

Ah yes, you are correct @jordwalke.

[jordwalke]

This is an interesting break, because they simply did opam upgrade, and it happened to bump their major version (they specified reason v >= 1.4) which means pushing a new major release in opam will cause a major version bump.

[yunxing]

I think we should still keep the keyword 'private' and 'public' so the parser can be compatible with them?

[SanderSpies]

@yunxing Could be possible, but right now we explicitly swap pri with private when moving from Reason to OCaml and vice versa. So that would need to be changed also.

[jordwalke]

So, to unbreak quickly, you can change this to use pri instead of private, and bump the minimum version in your opam file to be 1.7.2. I would like to preserve private in that situation though.

[jvillard]

Locking the version of reason to 1.4.0 also fixes the issue, so I think I'll do that for now (our CI uses 1.4.0).

[yunxing]

@SanderSpies Yea that's a challenge. We would need to modify the parser rules somehow.

@jvillard Locking the version to 1.4.0 would work, once we have the fix on our side we will push a new version, then I can send a patch to update (we need to update the version on package.json anyway).

[jvillard]

Thanks all for the very quick response!

[jvillard]

Note: I fixed it in infer (https://github.com/facebook/infer/commit/0b87ad23526c62827227e2c05f488c61cf318b75), but our previous stable release still specifies reason >= 1.4.0 and right now this doesn't compile.

I can reupload the release with a slightly different opam file that locks reason to 1.4.0. I thought I'd point out this use-case for the discussion whether to maintain backward compatibility.

[yunxing]

@jvillard Apologize for that. We are usually careful to maintain backward compatibility, but my brain wasn't running when merging the diff that changes the keyword. I will file a task to provide some backward-compatibility test.

This case is also what Yarn and other modern package managers are trying to solve -- a two file system which contains a requirement file specifying the range of dependency (e.g., 'reason >= 1.4.0'), plus a lock file that takes a snapshot of the dependency (e.g., yarn.lock).

Do you think reupload the release would be easy? I can quickly work on a fix and publish a new version today.

[jvillard]

Forgot to update this issue: I just released infer 0.9.5 to fix this issue on our side.

Glad to hear that the idea is to be backward compatible in the future.

[yunxing]

We do have some major syntax changes in mind that would break backward compatibility, but those would be signaled by a major version update.

[chenglou]

Done.