Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0007444OCamltypingpublic2016-12-30 07:142017-07-06 11:24
Reporterbmillwood 
Assigned Tofrisch 
PrioritynormalSeverityfeatureReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version4.03.0 
Target VersionFixed in Version4.06.0 +dev/beta1/beta2/rc1 
Summary0007444: module assignment enables use of deprecated values without a warning
DescriptionA module assignment can remove deprecation warnings from a value, by specifying a signature that does not have them. This means you can have a codebase where some value is deprecated, there are no usage warnings, but removing it breaks your build.

The behaviour I would expect would probably be that if a field of a module is necessary to satisfy the signature it is being constrained to, and that field is deprecated, then you should get a deprecation warning at that location.

You could also imagine refusing to match a deprecated value against an undeprecated signature, but that means adding deprecation attributes can break your build even if you don't treat deprecation warnings as errors, which is IMO also undesirable. (It's also presumably the case that you want module signatures to be permitted to *add* deprecation warnings, so that you can deprecate access to value x via module Y).
Steps To Reproducemodule X : sig
  val x : int [@@deprecated "[since 2016-12] x is bad"]
end = struct
  let x = 7
end

module Y : sig val x : int end = X
(* if we write module Y = X instead, then using Y.x generates a warning *)

let _ = Y.x (* no warning *)

(* or similarly: *)

module A : sig
  val x : int [@@deprecated "[since 2016-12] x is bad"]
end = struct
  let x = 7
end

module F(A : sig val x : int end) = struct
  let y = A.x
end

module B = F(A)
let _ = B.y (* no warning *)
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0017375)
frisch (developer)
2017-02-20 11:00

FWIW, I agree it would make sense to report the deprecation warning when a module inclusion check forgets the "deprecated" attribute.
(0017376)
frisch (developer)
2017-02-20 11:29
edited on: 2017-02-20 11:30

A POC implementation as as simple as:

diff --git a/parsing/builtin_attributes.ml b/parsing/builtin_attributes.ml
index bdbefcd..7280cbf 100755
--- a/parsing/builtin_attributes.ml
+++ b/parsing/builtin_attributes.ml
@@ -69,6 +69,13 @@ let check_deprecated loc attrs s =
   | Some txt ->
       Location.prerr_warning loc (Warnings.Deprecated (s ^ "\n" ^ txt))

+let check_deprecated_inclusion loc attrs1 attrs2 s =
+  match deprecated_of_attrs attrs1, deprecated_of_attrs attrs2 with
+  | None, _ | Some _, Some _ -> ()
+  | Some "", None -> Location.prerr_warning loc (Warnings.Deprecated s)
+  | Some txt, None ->
+      Location.prerr_warning loc (Warnings.Deprecated (s ^ "\n" ^ txt))
+
 let rec check_deprecated_mutable loc attrs s =
   match attrs with
   | [] -> ()
diff --git a/parsing/builtin_attributes.mli b/parsing/builtin_attributes.mli
index 9add637..47a9bdf 100755
--- a/parsing/builtin_attributes.mli
+++ b/parsing/builtin_attributes.mli
@@ -29,6 +29,8 @@


 val check_deprecated: Location.t -> Parsetree.attributes -> string -> unit
+val check_deprecated_inclusion:
+  Location.t -> Parsetree.attributes -> Parsetree.attributes -> string -> unit
 val deprecated_of_attrs: Parsetree.attributes -> string option
 val deprecated_of_sig: Parsetree.signature -> string option
 val deprecated_of_str: Parsetree.structure -> string option
diff --git a/typing/includemod.ml b/typing/includemod.ml
index deafeb0..cf4f8bb 100644
--- a/typing/includemod.ml
+++ b/typing/includemod.ml
@@ -57,6 +57,8 @@ let value_descriptions env cxt subst id vd1 vd2 =
   Cmt_format.record_value_dependency vd1 vd2;
   Env.mark_value_used env (Ident.name id) vd1;
   let vd2 = Subst.value_description subst vd2 in
+  Builtin_attributes.check_deprecated_inclusion vd2.val_loc vd1.val_attributes vd2.val_attributes
+    (Ident.name id);
   try
     Includecore.value_descriptions env vd1 vd2
   with Includecore.Dont_match ->


Easy to extend to other kinds of items than values. But the real trouble is how/where to report the warning. We have the current context, which is a kind of path (within this module inclusion check), but not the location of the module check itself.

(0017724)
frisch (developer)
2017-04-05 19:24

https://github.com/ocaml/ocaml/pull/1138 [^]

- Issue History
Date Modified Username Field Change
2016-12-30 07:14 bmillwood New Issue
2017-02-19 18:14 xleroy Severity minor => feature
2017-02-19 18:14 xleroy Status new => acknowledged
2017-02-19 18:14 xleroy Target Version => later
2017-02-20 11:00 frisch Note Added: 0017375
2017-02-20 11:29 frisch Note Added: 0017376
2017-02-20 11:30 frisch Note Edited: 0017376 View Revisions
2017-02-23 16:36 doligez Category OCaml general => -OCaml general
2017-02-27 15:49 doligez Assigned To => frisch
2017-02-27 15:49 doligez Status acknowledged => assigned
2017-02-27 15:49 doligez Category -OCaml general => typing
2017-02-27 15:49 doligez Target Version later =>
2017-04-05 19:24 frisch Note Added: 0017724
2017-07-06 11:24 frisch Status assigned => resolved
2017-07-06 11:24 frisch Fixed in Version => 4.06.0 +dev/beta1/beta2/rc1
2017-07-06 11:24 frisch Resolution open => fixed


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker