Comment author: @gasche

Is the third declaration you give correct? On my machine under 4.00.1, I cannot "get a warning back" for the same argument name, or by changing the function name, I need to change the argument name:

# let f x x = x;;
Warning 27: unused variable x.
val f : 'a -> 'b -> 'b = <fun>
# let f x x = x;;
val f : 'a -> 'b -> 'b = <fun>
# let g x x = x;;
val g : 'a -> 'b -> 'b = <fun>
# let f y y = y;;
Warning 27: unused variable y.
val f : 'a -> 'b -> 'b = <fun>

Then, outside the toplevel (with a file compiled with ocamlc -w +a -c foo.ml), I cannot reproduce this behaviour. The following declaration, in the same file, each result in an unused message warning:

  let f x x = x
  let f x x = x
  let f ~x ~x = x

PS: I think we should also have a warning for twice-bound variables in an abstraction sequence. It's a remark by Kaustuv Chaudhuri that it's not right that let f (x, x) = ... fails while the currified let f x x = ... is silently accepted -- except for the eventual unused variable warning.

Comment author: @sliquister

The trick is that the third declaration has a space before the let, so all the positions are shifted by one. It was visible in the terminal but maybe there was a problem in the copy pasting.

It does not happen in ocamlc because the positions are different, since the field Lexing.pos_cnum of positions increases for instance.
If you create a camlp4 plugin that generates the code you wrote by hand with the same position for all the identifiers, you will have zero warnings.

I don't know about what you say in PS. Why would we want a warning for a mistake that already has a warning? The problem is not the lack of warnings, it's people not using them. If anything, what you say would make me think that [let f (x, x) = ...] could compile with an unused variable warning.

Comment author: @alainfrisch

The easiest way to see the problem is using the toplevel:

This was reported in a different ticket (#5787) and has been fixed in the trunk.

One consequence is that warnings may not be given for the code generated by camlp4 extensions because the positions are usually duplicated.

Indeed, the new implementation of the warning rely on the locations, which might not work very well with Camlp4.

A previous implementation of the new warnings relied on mutable markers inserted in the declaration records themselves, but this was considered bad style. This would have avoided the reported problem.

Instead of physical locations, we could use some logical occurrence path, but this would be quite heavy. A simpler fix could be to rely on physical equality of declaration records, but I need to think more about it.

Comment author: @gasche

I think the "logical occurence path" idea is worth keeping on our mind, because it would solve other things too (eg. nail down the discussion of keeping locations in .cmi files, as those logical occurences are exactly what you need). That's one of the small-but-not-quite thing we would wish having an infinite supply of motivated interns for.

Comment author: @alainfrisch

No clear resolution plan.

@trefis, I think you might have addressed this one. Can you confirm.

I think that's right, at least I got some warnings in various ppxes when trying out my patch.