reasonml / reason

Simple, fast & type safe code that leverages the JavaScript & OCaml ecosystems
http://reasonml.github.io
MIT License
10.14k stars 428 forks source link

merlin reports warning 34 on .rei file in emacs #331

Closed jberdine closed 8 years ago

jberdine commented 8 years ago

Probably another aspect of the emacs mode not distinguishing between .re and .rei files is that merlin issues warning 34 (unused type) on items in .rei files. And there are warning 32, 34, 37, etc. reports in .re files that are clearly wrong considering the adjacent .rei file.

jordwalke commented 8 years ago

Just to confirm, this is with a correctly built project with Merlin file?

jberdine commented 8 years ago

Just to confirm, this is with a correctly built project with Merlin file?

I'm not sure, I will double-check when I can change branches without killing running tests and report back.

cristianoc commented 8 years ago

This happens with the Atom plugin as well.

1) Every value defined in a .rei files gives an unused value warning. 2) Values defined in both the .re and .rei give an unused value warning.

Marking as launch blocker as 1 is very noisy in the editor, so that one won't see real warnings.

jordwalke commented 8 years ago

@cristianoc: Did this happen with the latest bleeding edge? (At the bottom of the docs it says which repos to pin).

cristianoc commented 8 years ago

Yes on latest master. But it was the same on any previous versions.

On 21 Apr 2016, at 08:37, Jordan W notifications@github.com<mailto:notifications@github.com> wrote:

@cristianochttps://github.com/cristianoc: Did this happen with the latest bleeding edge? (At the bottom of the docs it says which repos to pin).

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHubhttps://github.com/facebook/Reason/issues/331#issuecomment-212767791

jordwalke commented 8 years ago

Yeah, I thought that the new merlin-extend would have fixed this. (To confirm, you had built everything and the .merlin file correctly lists the location of the directory containing source files via S path/to/src, and path to build artifacts using B path/to/buildArtifacts?

jordwalke commented 8 years ago

If that doesn't work, please let me know if adding the following to the .merlin will then make it work:

SUFFIX .re .rei

(The need to put SUFFIX should be temporary).

cristianoc commented 8 years ago

I had S and B, and I've added SUFFIX. No change (Atom plugin).

cristianoc commented 8 years ago

Something I've also noticed: jump to location from .re into .ml files used to work, now it only works from .re into .re files.

cristianoc commented 8 years ago

Removed launch blocker label as this seems to happen in normal .ml projects too. Looks like a general issue for merlin.

jordwalke commented 8 years ago

Looks like this is what happens on a pure ML project, I have confirmed. Your merlin file should probably disable the "unused variable" warnings 32 and 34.

jberdine commented 8 years ago

These warnings are not displayed with the current tooling for infer in ocaml, with the warning enabled. The solution is not to disable them. Something else is going on. Maybe the newer merlin is broken for both reason and ocaml?

jberdine commented 8 years ago

I just confirmed that this is still broken with the latest:

$ pwd /Users/jjb/src/reason $ git rev-parse HEAD a3c919738fbb0ff227b3c9f205b44957b003f744 $ opam pin list merlin.2.3.1 git git@github.com:the-lambda-church/merlin.git#df48da122f02e2276b14bcab58490350749215fa merlin_extend.0.2 git git@github.com:def-lkb/merlin-extend.git#ef634252a793542b05ec00a90f3c17de8fe0a357
reason.0.0.2 git /Users/jjb/src/reason

This is with a fully built infer tree, patched from @cristianoc's diff.

With these versions, this problem also occurs for the ocaml infer source.

This does not happen with the ocaml infer source using the latest released versions.

So, something is up with the latest merlin it seems.

jordwalke commented 8 years ago

Yes, this seems related to the current merlin version. I'll ping @def-lkb

let-def commented 8 years ago

The typechecking algorithm between the release and the master is a bit different. Global definitions were not considered as "warning sources" in the previous one. The problem with the new one is that merlin still has no idea of the uses, and so report everything as unused... I will fix that asap.

jordwalke commented 8 years ago

You are awesome, @def-lkb thank you!

let-def commented 8 years ago

I updated master, behavior should be closer to what is expected.

I would like to do the same checks as the compiler in a .re if Merlin can file the corresponding .rei for a .re, but that's long term :).

jordwalke commented 8 years ago

Works great for me!

cristianoc commented 8 years ago

Thanks!