Misplaced attributes for deprecation markers

[jonludlam]

I've just been testing odoc's support for 5.2 as contributed by @Octachron, which is working well and passing tests. However, one of our tests is producing a new warning that it wasn't before. The code in question looks like this:

...
module Top1 : sig
  [@@@deprecated "A"]

  (** Top-comment. *)
end
...

and I'm now getting a misplaced attribute warning:

File "cases/alerts.mli", line 20, characters 6-16:
20 |   [@@@deprecated "A"]
           ^^^^^^^^^^
Warning 53 [misplaced-attribute]: the "deprecated" attribute cannot appear in this context

I saw that @ccasin did a bunch of fixes in this area in #12451 so I had a look through that, but I'm afraid it's still not clear to me that this is expected behaviour. In odoc this is only generating a warning at compile time of the test, and the generated HTML still has the deprecated alert in it, so I think we might be missing a whitelisting of this attribute somewhere. If not, what's the right way to mark a whole module as deprecated?

[Octachron]

The correct syntax is

module Top1: sig
  ...
end[@@deprecated "A"]

or

module[@deprecated "A"] Top1: sig
  ...
end

The improved warning is reliable in the sense that the warning is now always produced whenever the compiler ignored a builtin attribute which was not the case before.

[ccasin]

The improved warning is reliable in the sense that the warning is now always produced whenever the compiler ignored a builtin attribute which was not the case before.

This is true for most attributes by design, but for deprecated the correctness of w53 is less certain (even after #12451). For most attributes we can decide not to issue w53 at the moment the compiler "uses" the attribute, but for deprecated the compiler "uses" it by just putting it into the environment where it may be found later by a check for issuing a deprecated warning. I did a fair amount of manual testing to ensure the places where we do/don't issue w53 for deprecated line up with the places that aren't/are checked later for deprecated warnings, but it is possible there is a bug here I will be happy to fix if needed. Such a bug would manifest by, either:

• The compiler issuing w53 for a deprecated attribute but then also issuing a deprecated warning for the place where this attribute appears, or
• The compiler issuing not issuing w53 for a deprecated attribute but also not issuing a deprecated warning when you use the definition the attribute is on.

In @jonludlam's original example I believe we are not in either of these cases - the compiler does issue w53 and does not issue a deprecated warning for uses of elements of this signature - but I will not be terribly surprised if I got this wrong.

[Octachron]

The example in question is indeed a case where the attribute was ignored.

[jonludlam]

The correct syntax is...

That makes sense for submodules, and we can change our test cases to look like that. I'm curious though, how would you go about deprecating an entire compilation unit?

[Octachron]

This is the case where a floating attribute in the "header" of the file (before non-attribute items) works:

[@@@deprecated "ok"]
val x: int

val x:int
[@@@deprecated "not_in_the_header"]

And there seems to be a warning 53 bug in this case, since the first form trigger a warning 53 in a ml only file whereas the warning doesn't trigger in the second case.

[gasche]

(See the related RFC ocaml/RFCs#26 that proposes giving a formal status to this idea of "header of a compilation unit".)

[ccasin]

And there seems to be a warning 53 bug in this case, since the first form trigger a warning 53 in a ml only file whereas the warning doesn't trigger in the second case.

Just wanted to acknowledge I've seen this and plan to fix it. Sorry for the delay, it's been a busy week.