Deprecation warning when checking signature inclusion#1138
Deprecation warning when checking signature inclusion#1138alainfrisch merged 17 commits intoocaml:trunkfrom
Conversation
Deprecation warning (3) is currently only reported when directly
accessing a component marked with the deprecated attribute; but it is
missed when we coerce the signature contaning the deprecated component
to a signature without the attribute.
This commit adds the required machinery to detect such cases
and report the same warning. (An alternative design could be
to introduce a new warning for that purpose.)
Some of the new machinery could be used for other purposes:
- During the inclusion check, keep the location that would used in
the error message if the check fails.
- Warnings can now hold extra "sub-locations" (and associated
messages).
|
@let-def Do you think that adding the definition of Location.loc to Warnings (with a re-export from Location) could create problem with ocaml-migrate-parsetree? |
|
No that seems fine. (The signature of Location will still be compatible) It sounds weird to have the definition of locations in Warnings module, even though I can understand why it is that way. |
|
Well, what is really weird is that Location (which should be rather generic) depends on Warning, and not the other way around. |
|
Ahah, I agree. From migrate-parsetree point I wasn't paying attention to the dep on Warnings. |
…eed to check twice.
alainfrisch
changed the title
|
Implementation extended to all kinds of component which support the deprecated attribute. Added a test and a changelog entry. Now ready for review. |
|
Is anyone interested to review this? |
typing/env.ml
Outdated
| begin match get_components desc1 with | ||
| Functor_comps f -> | ||
| Misc.may (!check_modtype_inclusion env mty2 p2) f.fcomp_arg; | ||
| Misc.may (!check_modtype_inclusion ~loc:(match loc with Some l -> l | None -> Location.none) env mty2 p2) f.fcomp_arg; |
There was a problem hiding this comment.
I think this line is too long (and a bit messy). Could you lift the (match loc ...) bit out to it's own binding.
typing/env.ml
Outdated
| begin match get_components desc1 with | ||
| Functor_comps f -> | ||
| Misc.may (!check_modtype_inclusion env mty2 p2) f.fcomp_arg; | ||
| Misc.may (!check_modtype_inclusion ~loc:(match loc with Some l -> l | None -> Location.none) env mty2 p2) f.fcomp_arg; |
typing/includeclass.ml
Outdated
| loc | ||
| cty1.clty_attributes cty2.clty_attributes | ||
| (Path.last cty1.clty_path); | ||
|
|
There was a problem hiding this comment.
Nitpick: I find this blank line makes it hard to see what is actually in class_type_declarations. I think leaving blank lines in the middle of definitions is against the unwritten style guide of the compiler.
typing/includecore.ml
Outdated
| loc | ||
| cd1.cd_attributes cd2.cd_attributes | ||
| (Ident.name cd1.cd_id); | ||
|
|
There was a problem hiding this comment.
Nitpick: Unnecessary blank line hinders readability.
typing/includecore.ml
Outdated
| loc | ||
| ld1.ld_attributes ld2.ld_attributes | ||
| (Ident.name ld1.ld_id); | ||
|
|
typing/includecore.ml
Outdated
| loc | ||
| decl1.type_attributes decl2.type_attributes | ||
| name; | ||
|
|
typing/includemod.ml
Outdated
| loc | ||
| md1.md_attributes md2.md_attributes | ||
| (Ident.name id1); | ||
|
|
There was a problem hiding this comment.
Unecessary blank line hinders readability
typing/includemod.ml
Outdated
| loc | ||
| info1.mtd_attributes info2.mtd_attributes | ||
| (Ident.name id); | ||
|
|
| let type_declarations env id decl1 decl2 = | ||
| type_declarations env [] Subst.identity id decl1 decl2 | ||
| let modtypes ~loc env mty1 mty2 = modtypes ~loc env [] Subst.identity mty1 mty2 | ||
| let signatures env sig1 sig2 = |
There was a problem hiding this comment.
It seems inconsistent for signatures not to have a loc parameter.
There was a problem hiding this comment.
Includemod.signatures is only used in context which could not trigger the warning, I believe (in {opt,}{compile,toploop}.ml). Moreover the resulting coercion is ignored in those cases. Perhaps it would be better to expose a more specific function (better name and a unit result) to cover those cases (do you happen to remember why we need to check self-inclusion of signatures e.g. in compiler.ml?). But this is not really related to this PR anyway.
There was a problem hiding this comment.
(Ah, this could be used to mark the exported declarations as being used, to avoid the unused warnings.)
|
I left a few style comments, but it all looks correct to me. |
|
Thanks @lpw25! |
4 participants
https://caml.inria.fr/mantis/view.php?id=7444
Deprecation warning (3) is currently only reported when directly
accessing a component marked with the deprecated attribute; but it is
missed when we coerce the signature containing the deprecated component
to a signature without the attribute.
This PR adds the required machinery to detect such cases
and report the same warning. An alternative design could be
to introduce a new warning for that purpose. Do people believe one
should reuse the existing warning, or introduce a new one?
Another design choice is that only the presence of the deprecated attribute is checked. Changing the payload of the attribute does not trigger the warning.
Some of the new machinery could be used for other purposes:
During the inclusion check, keep the location that would used in
the error message if the check fails.
Warnings can now hold extra "sub-locations" (and associated
messages).
One case of a missing attribute was found on StringLabels.copy (after
bootstrapping the compiler).
All kinds of deprecated attributes are supported (on values, types, constructors, class/class types, module/module types; and also the "deprecated_mutable" attribute on record labels).