Add an “unsafe” warning and annotation#730
Add an “unsafe” warning and annotation#730nicoonoclaste wants to merge 11 commits intoocaml:trunkfrom nicoonoclaste:unsafe
Conversation
typing/typedecl.ml
Outdated
| | [] -> | ||
| raise (Error(valdecl.pval_loc, Val_in_structure)) | ||
| | _ -> | ||
| let msg = Printf.sprintf "the signature for extern %s cannot be checked" valdecl.pval_name.txt in |
There was a problem hiding this comment.
for "external"?
Should this be restricted to type-checking implementation, or should this also fire when type-checking signature? (No strong opinion.)
|
I think this will be used by enabling the warning (as error) in a given project, and locally disabling it, to clearly identity sections that use unsafe features. I'm not sure that |
|
Do you also propose to mark some functions and modules from the stdlib as unsafe? |
|
@alainfrisch Yes, I'm planning to mark some functions & modules, |
|
I guess this is because stdlib is built with boot/ocamlc in a simple |
|
@alainfrisch I had a |
That's exactly what I had in mind when I wrote “enforce that a certain codebase does not accidentaly use unsafe features”. Feel free to chime in if you can think of a clearer wording. |
This is sub-optimal: Obj.reachable_words (for instance) is perfectly type-safe.
|
Nobody has offered any argument against this PR and I would like to push it forward. @nbraud could you rebase to current trunk? |
|
The equivalent of https://caml.inria.fr/mantis/view.php?id=7444 seems even more important here. It was also discussed during the previous meeting of SecurOCaml to generalize a bit the treatment of deprecated/unsafe to allow exposing arbitrary "tags" and guarantee that a given piece of code does not refer to any component marked with some specified tags. Unsafe and deprecated would be such tags, but one might want to be more fine-grained, and mark also code performing I/O (and even distinguish read/write operations), code raising exceptions, code using the generic comparison, code relying on GC features such as weak pointers or finalizers, etc. (Some language features could "raise" such tags themselves.) So perhaps before merging this PR one could discuss such a possible generalization. |
|
@nbraud Are you interested to work on a slightly more general proposal, which would generalize notions of "deprecated" and "unsafe"? |
|
@alainfrisch Possibly. What did you have in mind? Sorry for letting this bitrot so long, I was not very available in a very long while. :( |
Basically what I described in my note above (#730 (comment)), i.e. something which would generalize both "deprecated" and "unsafe" (some more built-in such "tags", and also user-defined ones). |
|
I've started to work on generalizing "deprecated" to more general "alerts" that can be triggered when using specially marked components, see https://github.com/alainfrisch/ocaml/tree/alerts |
|
Cf #1804. |
* Add printer for Value_approximation * Fix for continuations of inlined functions
This is a proposal for implementing an
unsafewarning, similar todeprecated, as suggested by @alainfrisch in #724.Description
Warning 62 (“unsafe”) is added, and triggers under the following conditions:
external;ocaml.unsafe, as withocaml.deprecated.The implementation is very similar to the implementation of
ocaml.deprecated, modulo some minor refactoring (calls toBuiltin_attributes.check_deprecatedwere replaced by calls tocheck_attr, which simply calls bothcheck_deprecatedand the newcheck_unsafe).Some minor changes were required in the stdlib,
Makefiles and testsuite due to the pervasive use of-warn-error A.Motivation
There are two main reasons for implementing this:
This makes it very easy (by making this warning an error) to enforce that a certain codebase does not accidentaly use unsafe features, either through the stdlib (
Obj,Marshal,unsafe_*) or by defining anexternal.Even outside of a codebase with an explicit policy regarding the use of unsafe features, a warning that is enabled by default will at least make the user aware of what they are doing, and hopefully consider safer/saner alternatives.
Notes
As mentionned by @mshinwell, many libraries expose their own
externals (with a sound type) in their API (which is useful in the context of bytecode execution). As such, this should only raise a warning at the definition point of theexternal(unless it is annotated withocaml.unsafe).