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

warning 50 (documentation comments) too strict #6916

Closed
vicuna opened this Issue Jun 25, 2015 · 6 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

vicuna commented Jun 25, 2015

Original bug ID: 6916
Reporter: @ygrek
Status: resolved (set by @damiendoligez on 2017-02-24T12:13:20Z)
Resolution: won't fix
Priority: normal
Severity: tweak
Version: 4.02.2
Target version: later
Category: lexing and parsing
Monitored by: @gasche

Bug description

New warning in 4.02.2 leads to less compact code.

Consider:

type t1 = {
a:int; (** a )
b:int; (
* b *)
}

type t2 = <
a:int; (** a )
b:int; (
* b *)

module type T = sig
val a : unit -> int (** a )
val b : unit -> int (
* b *)
end

This results in

File "a.ml", line 7, characters 9-17:
Warning 50: unattached documentation comment (ignored)
File "a.ml", line 8, characters 9-17:
Warning 50: unattached documentation comment (ignored)
File "a.ml", line 12, characters 20-28:
Warning 50: ambiguous documentation comment

So for the records tail-placement of comments is ok, but not for objects nor module types. Can it be fixed so that all these examples be accepted?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 25, 2015

Comment author: @lpw25

This is the intended behaviour.

Firstly, documentation comments on the methods of object types are not supported (and were not supported by ocamldoc). Comments can be added to object definitions, classes and class types, but not to object types.

The comment attachment rules are indeed different for sub-items (i.e. record fields and variant constructors) and items (i.e. everything else). For sub-items comments must appear after the sub-item and so there is no potential for ambiguity. For items comments may come before or after the item so blank lines are required to avoid ambiguity.

In your signature example, it is not clear that (** a *) should be attached to a rather than b, so a blank line is required after the comment to remove this ambiguity.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 25, 2015

Comment author: @ygrek

Firstly, documentation comments on the methods of object types are not supported (and were not supported by ocamldoc).

This is what I get in ocamldoc 4.02.2 html output, just as expected :

type t1 = { a : int; (* a )
b : int; (
b *)

}
type t2 = < a : int; (* a )
b : int; (
b *)

As for the anbiguous comments, why doesn't compiler use same disambiguation rules as ocamldoc? Otherwise I would expect warning 50 to be split in two, so that it is possible to silence ambiguous warning, but continue get warnings on unattached comments. The latter one is useful, but the first one is compiler telling how to indent code - too invasive.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 26, 2015

Comment author: @lpw25

This is what I get in ocamldoc 4.02.2 html output, just as expected

Sorry yes, I was thinking of polymorphic variants. However, ocamldoc's behaviour here is inconsistent and object types are deliberately not handled by the ocamlc parser.

As for the anbiguous comments, why doesn't compiler use same disambiguation rules as ocamldoc?

There were a lot of complaints that ocamldoc's comment attachment rules are too complicated. The attachment rules implemented in the ocamlc parser are mostly backwards compatible but they have been slightly simplified and made more consistent.

Otherwise I would expect warning 50 to be split in two, so that it is possible to silence ambiguous warning, but continue get warnings on unattached comments.

The support for attaching comments to the AST in the ocamlc parser is not for ocamldoc. It is so that new documentation tools can be developed which operate on .cmt(i) files. If you do not intend to use these tools you should leave warning 50 turned off. If you do intend to use them then both ambiguous comments and unattached comments should be fixed as they will not produce the expected behaviour from these tools. So I do not see the benefit of producing separate warnings.

The latter one is useful, but the first one is compiler telling how to indent code - too invasive.

ocamldoc's comment attachment rules have always been sensitive to blank lines, this is just another instance of that. If you do not like blank-line sensitivity you can instead use attributes. The ocamlc parser produces the same output for:

module type T = sig
val a : unit -> int (** a *)

val b : unit -> int (** b *)

end

and

module type T = sig
val a : unit -> int [@@doc "a"]
val b : unit -> int [@@doc "b"]
end

(Again this only applies to tools based on the new support for attaching comments in the ocamlc parser. It does not apply to ocamldoc).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 22, 2015

Comment author: @damiendoligez

Nitpick:

val a : unit -> int [@@doc "a"]

If you really want the same output from the compiler, this should be [@@ocaml.doc "a"]. But the tools will treat @@doc in the same way as @@ocaml.doc, so they are equivalent.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Aug 27, 2015

Comment author: @oandrieu

As for the anbiguous comments, why doesn't compiler use same disambiguation rules as ocamldoc?

There were a lot of complaints that ocamldoc's comment attachment rules are too complicated. The attachment rules implemented in the ocamlc parser are mostly backwards compatible but they have been slightly simplified and made more consistent.

Not a big fan either of these new rules for attaching comments …

Rules for ocamldoc in .ml files were pretty straightforward IMHO (comment before the element, except for records and variants).

Now in my case I have zillions of warning 50 and confusing ocaml.doc attributes because code like this:

(** comment 1 *)
let f x = ...
(** comment 2 *)
let g x = ...

results in f having both comments.

(But I agree the rules for .mli files were indeed a bit baroque).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Aug 27, 2015

Comment author: @lpw25

Now in my case I have zillions of warning 50 and confusing ocaml.doc attributes > because code like this:
(** comment 1 )
let f x = ...
(
* comment 2 *)
let g x = ...

results in f having both comments.

I agree that the case for .ml files is less ambiguous because almost no one would try to put the comment for a let after it. However, I think that it is worth the mild inconvenience of adding a newline in order to keep the rules the same for all definitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.