0006202: Controlling warnings locally

(0010426)
hcarty (reporter)
2013-10-03 21:14

I think this would be very useful for the reasons you mentioned. It could also be useful for open!-like use of the M.(...) syntax.

(0010427)
garrigue (manager)
2013-10-04 01:16

I think this is a good idea, but we must be very careful about readability and clarity of the semantics.
This changes the compiler behaviour, so you want to now exactly the scope of the change, at first sight.
From this point of view, your "begin" example would nice.
Having the attribute after the expression might make this less clear.

(0010428)
frisch (developer)
2013-10-04 10:25

Commit 14214 on trunk: the 'warning' attribute is interpreted on expressions (the scope is restricted to the target expression) and in floating signature/structure attributes (with a global scope, not even restricted to the current signature or structure). There is a subtle interaction between the two: when an expression has a 'warning' attribute, the state of warnings before the expression is type-checked is restored after the expression has been type-checked, which cancels the effect of floating attributes within the expression (in a local module expression under that expression).

Notes:

- The 'warning' attribute on expression will probably be used with the form:

     begin[@warning "..."]
     ...
     end

   but this is strictly equivalent to the postfix notation:

     (...) [@warning "..."]

- The 'warning' attribute is not interpreted on other syntactic categories (patterns, module expressions, class expressions).

- Floating item attributes can appear at the beginning of a signature or structure (hence at the beginning of a compilation unit), or after a ";;" token within a structure or signature. They can now also be used in the top-level, which means one can simply type [@@warning "..."];; as a toplevel phrase.

- The commit also adds a new warning to report in illegal payload for the 'warning' attribute.

Comments are welcome!

(0010429)
lpw25 (developer)
2013-10-04 12:49

As a side note, we should start putting a list of these "reserved" attributes somewhere so that people know not to use them for their own extensions.

(0010430)
guesdon (manager)
2013-10-04 12:55

A warning could be added to indicate that some warnings were turned off/on locally, so that using this new warning can be used on the command line to remove all locally turned on/off warnings.

+1 on having the possibility to have attributes before items they refer to, not only after.

(0010431)
frisch (developer)
2013-10-04 13:21

> we should start putting a list of these "reserved" attributes

Do you think we should use a special namespace convention for attributes which are given a special meaning by the OCaml compiler itself? E.g. [@ocaml.deprecated], [@ocaml.warning] ?

(0010945)
doligez (administrator)
2014-02-19 15:32

+1000 on Maxence's idea of a new warning that gets triggered every time this feature is used.
Even if we don't do it now, I'm sure the Frama-C developers will demand it.

(0010946)
hcarty (reporter)
2014-02-19 15:51

A special or reserved attribute prefix for attributes provided by the OCaml compiler is a useful idea. These attributes don't require an extra ppx - it's nice to immediately know which attributes can be used without external tools.

(0011268)
frisch (developer)
2014-04-14 17:19

Attribute renamed to ocaml.warning.

The syntax of floating attributes as changed as well:

[@@@ocaml.warning "..."]

(0011628)
frisch (developer)
2014-06-02 19:03
edited on: 2014-06-03 11:00

I'm a little bit unsatisfied with local control of warnings. While it is well-defined to control warnings for a compilation unit, it is less clear what the exact meaning is for even more local control. Concretely, many warnings are issued by the type-checker in a final pass (e.g. unused stuff warnings), not while type-checking an inner component. Currently [@@@ocaml.warning] changes the set of active warnings while type-checking such inner components, so this does not interact nicely. A better defined behavior would be to filter warnings according to their location, independently of where in the type-checker code they are issued. This is a bit more complex to implement, though.

Typically, functions such as Location.prerr_warning should take an extra Warnings.state parameter. Alternatively, it could make sense to avoid keeping warning configuration in a global state and store it in the Env.t instead (which would force to think about the relevant scope when issuing a warning or checking that one warning is enabled).

edit: there is already some provision so that when a delayed check is run, the warning configuration from the point it was registered is restored.

(0011858)
doligez (administrator)
2014-07-16 17:32

This feature interferes badly with the OCAMLPARAM feature. We need to give OCAMLPARAM precedence over local changes.

(0011881)
frisch (developer)
2014-07-17 16:39

Keeping this opened. The implementation needs to be improved and the interaction with OCAMLPARAM discussed. Let's consider the existing feature as experimental.

(0017519)
frisch (developer)
2017-03-01 16:19

I'm not sure that OCAMLPARAM should take precedence over local controls.

(0018515)
frisch (developer)
2017-10-09 15:58

The feature works much better in 4.06. I'm thus closing this issue.

Damien: you mentioned that OCAMLPARAM should take precedence. But I see OCAMLPARAM as a way to inject flags at the level of the command-line (without having to patch build systems), and with this interpretation it should not take precedence over attributes. Feel free to open another ticket if you feel strongly about your point of view and would like us to discuss further.

Issue History
Date Modified	Username	Field	Change
2013-10-03 17:26	frisch	New Issue
2013-10-03 21:14	hcarty	Note Added: 0010426
2013-10-04 01:16	garrigue	Note Added: 0010427
2013-10-04 10:25	frisch	Note Added: 0010428
2013-10-04 12:49	lpw25	Note Added: 0010429
2013-10-04 12:55	guesdon	Note Added: 0010430
2013-10-04 13:21	frisch	Note Added: 0010431
2013-10-04 13:21	frisch	Assigned To	=> frisch
2013-10-04 13:21	frisch	Status	new => assigned
2014-02-19 15:32	doligez	Note Added: 0010945
2014-02-19 15:51	hcarty	Note Added: 0010946
2014-04-14 17:19	frisch	Note Added: 0011268
2014-06-02 19:03	frisch	Note Added: 0011628
2014-06-02 19:09	frisch	Note Edited: 0011628	View Revisions
2014-06-03 11:00	frisch	Note Edited: 0011628	View Revisions
2014-07-16 17:22	doligez	Relationship added	related to 0006483
2014-07-16 17:32	doligez	Note Added: 0011858
2014-07-16 17:32	doligez	Target Version	=> 4.02.0+dev
2014-07-17 16:39	frisch	Note Added: 0011881
2014-07-17 16:39	frisch	Target Version	4.02.0+dev => 4.02.1+dev
2014-09-04 00:25	doligez	Target Version	4.02.1+dev => undecided
2014-09-15 23:20	doligez	Severity	minor => feature
2014-09-15 23:20	doligez	Target Version	undecided =>
2017-02-23 16:36	doligez	Category	OCaml general => -OCaml general
2017-03-01 15:58	doligez	Category	-OCaml general => misc
2017-03-01 16:19	frisch	Note Added: 0017519
2017-10-09 15:58	frisch	Note Added: 0018515
2017-10-09 15:58	frisch	Status	assigned => resolved
2017-10-09 15:58	frisch	Fixed in Version	=> 4.06.0 +dev/beta1/beta2/rc1
2017-10-09 15:58	frisch	Resolution	open => fixed

Notes
(0010426) hcarty (reporter) 2013-10-03 21:14	I think this would be very useful for the reasons you mentioned. It could also be useful for open!-like use of the M.(...) syntax.

(0010427) garrigue (manager) 2013-10-04 01:16	I think this is a good idea, but we must be very careful about readability and clarity of the semantics. This changes the compiler behaviour, so you want to now exactly the scope of the change, at first sight. From this point of view, your "begin" example would nice. Having the attribute after the expression might make this less clear.

(0010428) frisch (developer) 2013-10-04 10:25	Commit 14214 on trunk: the 'warning' attribute is interpreted on expressions (the scope is restricted to the target expression) and in floating signature/structure attributes (with a global scope, not even restricted to the current signature or structure). There is a subtle interaction between the two: when an expression has a 'warning' attribute, the state of warnings before the expression is type-checked is restored after the expression has been type-checked, which cancels the effect of floating attributes within the expression (in a local module expression under that expression). Notes: - The 'warning' attribute on expression will probably be used with the form: begin[@warning "..."] ... end but this is strictly equivalent to the postfix notation: (...) [@warning "..."] - The 'warning' attribute is not interpreted on other syntactic categories (patterns, module expressions, class expressions). - Floating item attributes can appear at the beginning of a signature or structure (hence at the beginning of a compilation unit), or after a ";;" token within a structure or signature. They can now also be used in the top-level, which means one can simply type [@@warning "..."];; as a toplevel phrase. - The commit also adds a new warning to report in illegal payload for the 'warning' attribute. Comments are welcome!

(0010429) lpw25 (developer) 2013-10-04 12:49	As a side note, we should start putting a list of these "reserved" attributes somewhere so that people know not to use them for their own extensions.

(0010430) guesdon (manager) 2013-10-04 12:55	A warning could be added to indicate that some warnings were turned off/on locally, so that using this new warning can be used on the command line to remove all locally turned on/off warnings. +1 on having the possibility to have attributes before items they refer to, not only after.

(0010431) frisch (developer) 2013-10-04 13:21	> we should start putting a list of these "reserved" attributes Do you think we should use a special namespace convention for attributes which are given a special meaning by the OCaml compiler itself? E.g. [@ocaml.deprecated], [@ocaml.warning] ?

(0010945) doligez (administrator) 2014-02-19 15:32	+1000 on Maxence's idea of a new warning that gets triggered every time this feature is used. Even if we don't do it now, I'm sure the Frama-C developers will demand it.

(0010946) hcarty (reporter) 2014-02-19 15:51	A special or reserved attribute prefix for attributes provided by the OCaml compiler is a useful idea. These attributes don't require an extra ppx - it's nice to immediately know which attributes can be used without external tools.

(0011268) frisch (developer) 2014-04-14 17:19	Attribute renamed to ocaml.warning. The syntax of floating attributes as changed as well: [@@@ocaml.warning "..."]

(0011628) frisch (developer) 2014-06-02 19:03 edited on: 2014-06-03 11:00	I'm a little bit unsatisfied with local control of warnings. While it is well-defined to control warnings for a compilation unit, it is less clear what the exact meaning is for even more local control. Concretely, many warnings are issued by the type-checker in a final pass (e.g. unused stuff warnings), not while type-checking an inner component. Currently [@@@ocaml.warning] changes the set of active warnings while type-checking such inner components, so this does not interact nicely. A better defined behavior would be to filter warnings according to their location, independently of where in the type-checker code they are issued. This is a bit more complex to implement, though. Typically, functions such as Location.prerr_warning should take an extra Warnings.state parameter. Alternatively, it could make sense to avoid keeping warning configuration in a global state and store it in the Env.t instead (which would force to think about the relevant scope when issuing a warning or checking that one warning is enabled). edit: there is already some provision so that when a delayed check is run, the warning configuration from the point it was registered is restored.

(0011858) doligez (administrator) 2014-07-16 17:32	This feature interferes badly with the OCAMLPARAM feature. We need to give OCAMLPARAM precedence over local changes.

(0011881) frisch (developer) 2014-07-17 16:39	Keeping this opened. The implementation needs to be improved and the interaction with OCAMLPARAM discussed. Let's consider the existing feature as experimental.

(0017519) frisch (developer) 2017-03-01 16:19	I'm not sure that OCAMLPARAM should take precedence over local controls.

(0018515) frisch (developer) 2017-10-09 15:58	The feature works much better in 4.06. I'm thus closing this issue. Damien: you mentioned that OCAMLPARAM should take precedence. But I see OCAMLPARAM as a way to inject flags at the level of the command-line (without having to patch build systems), and with this interpretation it should not take precedence over attributes. Feel free to open another ticket if you feel strongly about your point of view and would like us to discuss further.