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

module assignment enables use of deprecated values without a warning #7444

Closed
vicuna opened this issue Dec 30, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Dec 30, 2016

Original bug ID: 7444
Reporter: bmillwood
Assigned to: @alainfrisch
Status: resolved (set by @alainfrisch on 2017-07-06T09:24:22Z)
Resolution: fixed
Priority: normal
Severity: feature
Version: 4.03.0
Fixed in version: 4.06.0 +dev/beta1/beta2/rc1
Category: typing
Monitored by: bmillwood

Bug description

A 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 reproduce

module 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 *)

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 20, 2017

Comment author: @alainfrisch

FWIW, I agree it would make sense to report the deprecation warning when a module inclusion check forgets the "deprecated" attribute.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 20, 2017

Comment author: @alainfrisch

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 5, 2017

Comment author: @alainfrisch

#1138

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.