Clean up and extend support for ocaml.warning/ocaml.warnerror/ocaml.ppwarning attributes by alainfrisch · Pull Request #1248 · ocaml/ocaml

alainfrisch · 2017-07-17T22:28:26Z

This commit includes the following changes:

Some internal cleanup in the implementation of ocaml.warning/ocaml.warnerror.
Extend the support for those attributes in all attribute contexts. For instance, with this PR, [@ocaml.warning "-3"] can be used on a type expression. See Avoid "internal" deprecation warnings #1237 for a discussion on the need for this feature.
Re-implement the support for ocaml.ppwarning so that the warning raised by this attribute
are properly scoped by surrounding ocaml.warning/ocaml.warnerror attribute. This benefits from the machinery put in place for the bullet above, and the new implementation is simpler and more efficient than the previous one (dedicated traversal over the AST). Fixes https://caml.inria.fr/mantis/view.php?id=7361

This is now ready for review.

alainfrisch · 2017-07-18T12:00:41Z

Ok, the work done to support ocaml.warning is all attribute contexts also made it easy to reimplement support for ocaml.ppwarning in a way which respect the scopes of ocaml.warning attributes.

Now ready for reviews. Volunteers are welcome! (@aantron : hint hint)

trefis · 2017-07-18T12:29:11Z

Hi,

I like the general idea of this PR (i.e. adding support for such attributes in all contexts), as well as the new warning_scope function (although I'm not too fond of the name, but I'm sure various people will have suggestions about that) which I find much nicer than the explicit enter_scope and leave_scope functions it replaces.

However, the new Warnings.mk_lazy functions leaves me uneasy. I understand why it's there, but it feels very error prone : it seems easy to forget to use it in practice and easy to overlook during review.
In fact I already don't know if you've added it to all the places where it is needed. I assume you did, but apart from grepping for lazy and checking all the use cases, there's no way for me to know if there is no bug in this PR.
Perhaps it is a good time to part ways with this particular instance of global state and thread the warning environments through all the places where it's needed? This could be done as a separate, preliminary, PR.
Note that changes like the one I just proposed do/would make the life of merlin's developers and maintainers marginally nicer :)

trefis · 2017-07-18T12:39:49Z

testsuite/tests/warnings/w03.reference

+File "w03.ml", line 19, characters 9-12:
+Warning 3: deprecated: X.t
+File "w03.ml", line 21, characters 15-17:
+Warning 3: deprecated: t7


I don't think you meant to commit this file.

Well spotted, thanks!

alainfrisch · 2017-07-18T13:03:04Z

Perhaps it is a good time to part ways with this particular instance of global state and thread the warning environments through all the places where it's needed?

To be checked, but there is probably already an Env.t available everywhere we need it, and we could use it to thread the warnings information as well.

I agree this would be a nice cleanup.

However, I think the PR in its current state adds useful features and also remove existing drawbacks. The worse that can happen if I forgot to deal with a lazy is a scoping problem for some ocaml.warning attribute which is not the end of the world (and not even a regression, since those attributes were ignored in most contexts). I checked all the lazy in Typeclass, and I don't believe lazyness is used in other places. Moreover, I've provided a rather extensive test (which did catch the problem with lazy, initially).

So unless someone plans to work on the suggested cleanup in the short term, I think it is better to record it for future work and accept this PR.

alainfrisch · 2017-07-18T15:22:04Z

To be checked, but there is probably already an Env.t available everywhere we need it, and we could use it to thread the warnings information as well.

Actually, it's not the case. We'd need to have access to the warning configuration everywhere a warning can be raised, which would create a rather invasive change. Note that even the lexer can raise warnings (although they are currently not controlled by ocaml.warning).

trefis · 2017-07-18T16:00:10Z

To be checked, but there is probably already an Env.t available everywhere we need it, and we could use it to thread the warnings information as well.

Actually, it's not the case. We'd need to have access to the warning configuration everywhere a warning can be raised, which would create a rather invasive change. Note that even the lexer can raise warnings (although they are currently not controlled by ocaml.warning).

To be fair the flow of Env.ts is not inherently the same as the one for warning scopes, so I'm not sure it would have been a good idea to push the warnings state into Env.t regardless.
(Then again, the flags field in Env.t already feels a bit meh.)

I do agree the change is invasive yes, but it does still feel beneficial to me.

The worse that can happen if I forgot to deal with a lazy is a scoping problem for some ocaml.warning attribute which is not the end of the world (and not even a regression, since those attributes were ignored in most contexts).

I do not quite agree with the reasoning here. Here if a bug creeps in it doesn't seem unrealistic that we would turn off some warning in a place where we're not supposed to do so, that seems bad.
The fact that you have made no mistake here doesn't mean that no one will ever make one, and your tests only covers constructions of the language which already exist. I'm worried about new code being written which doesn't go through Warnings.mk_lazy and doesn't test the interaction with warning attributes.

My personal feeling is that this PR is adding an easy way to write bogus code and it seems like we could prevent that with some effort. Depending on your definition of short-term ("before the 4.06 freeze"?) I wouldn't mind waiting to see if a kind soul is willing to at least experiment with threading the warnings context explicitly.

alainfrisch · 2017-07-18T16:12:41Z

My personal feeling is that this PR adding an easy way to write bogus code

I don't think it's a fair statement. The type-checker is already full of global state, and even before this PR, we already used the same technique to support ocaml.warning. It is just that the PR extends the support for ocaml.warning to classes, where the type-checker uses lazyiness (in a relatively well controlled place).

If this PR covered all cases except for class fields, would you still consider that its inclusion is conditioned to the big refactoring?

trefis · 2017-07-19T08:23:57Z

The type-checker is already full of global state

No one is denying that :)
At the same time, bugs have, in the past, crept into the typechecker as a result of global state and human forgetfulness.

we already used the same technique to support ocaml.warning

That's a fair point, indeed as you already noted this PR doesn't introduce any regression. It simply increases the number of places where errors can be made.

If this PR covered all cases except for class fields, would you still consider that its inclusion is conditioned to the big refactoring?

Oh, I'd rather we merged the whole thing rather than omitting support for classes. Omitting class fields in this PR wouldn't make the implementation fundamentally safer, just less complete.
(Also, someone might come at a later date to add support there, forgetting both about tests and the lazy. This seems worse than accepting your PR which was carefully tested and does handle lazyness.)

I still think it'd be good if the cleanup was done in the not too distant future though.

alainfrisch · 2017-07-20T07:31:44Z

To be fair the flow of Env.ts is not inherently the same as the one for warning scopes

Can you elaborate on that?

What I had in mind was changing from

val warning_attribute: Parsetree.attribute -> unit
val warning_scope: Parsetree.attributes -> (unit -> 'a) -> 'a

to

val warning_attribute: Env.t -> Parsetree.attribute -> Env.t
val warning_scope: Env.t -> Parsetree.attributes -> (Env.t -> 'a) -> 'a

Adapting call sites for these function to these new signature should be easy. Then we only need to ensure that the env is passed in all contexts where a warning can be raised.

Anyway, I think this is for later.

=====

@garrigue Do you have an opinion on the proposal?

nojb · 2017-07-20T07:44:36Z

Wouldn't warning_scope no longer be necessary with this formulation ? i.e., something like warning_scope env a f = f (warning_attribute env a) ?

alainfrisch · 2017-07-20T08:01:20Z

Yes, with a fold (warning_scope takes multiple attributes).

trefis · 2017-07-20T09:07:15Z

To be fair the flow of Env.ts is not inherently the same as the one for warning scopes

Can you elaborate on that?

Well, the general vague idea is that warnings should flow down to subtrees only, while the environment might sometimes pass from one branch to the next.
Although I'm not sure it will be a problem in practice.

I'm not that familiar with the typechecker (so I have probably missed something when trying to follow the code) but, after a quick look inside typecore.ml, I wonder if the following could be problematic: in type_cases one builds pat_env_list, which is a (Typedtree.pattern * (Env.t * _)) list where for each element (pat, (env, _)) of the list env seems to be the env obtained after typechecking the pattern.
We then use that environment to typecheck both the guard and the body of the related branch.
Could warnings attributes attached to the pattern somehow leak into the body of that branch?
I don't know, it would have to be tried (or looked at by someone who understands the code better than me).

That being said, storing the warnings in the environment has the nice side effect that they would be available at a later stage when translating from typedtree to lambda. I imagine this could prove useful.

Octachron · 2017-08-24T13:37:08Z

typing/typemod.ml

   making them abstract otherwise. *)

 let rec approx_modtype env smty =
+  (* Need to process warning attributes here ?? *)


Should warnings rather be disabled when computing approximative module types (i.e. in approx_* functions) since those approximation are only used to initialize the typing of recursive module types and warnings can be emitted once the full module types have been computed?

Yes, indeed. This would fall in the scope of #1191 I think.

Well, in #1191, the call to approx_* function is not protected against warnings, but it should probably be if the current PR is merged before.

alainfrisch · 2017-08-30T21:27:12Z

Rebased, and fixed the test.

@Octachron Since you started to comment on the PR, would you like to review it?

Octachron · 2017-08-31T17:49:55Z

Certainly, I would try to find the time for a full review this week.

alainfrisch · 2017-09-06T13:54:04Z

Gentle ping to @Octachron.

trefis

Stylistic disagreement aside, the code looks correct to me.

Octachron

The PR looks good but I had some suggestions for minor potential tweaks.

One more potential tweak might be the Typetexp.transl_type_param function that does not take in account the attributes on the type parameters. Even, it is currently not possible to emit a warning here, I wonder if it may not be better to enclose it in a warning scope already?

Octachron · 2017-09-10T19:02:51Z

manual/manual/refman/exten.etex

+ (such as an expression, or a type expression)
+ in which case its scope is limited to that item.
 Note that it is not well-defined which scope is used for a specific
 warning.  This is implementation dependant and can change between versions.


I think it would be a good idea to mention here that some warnings are not affected by such attributes. In particular, lexical warnings (latin 1 identifiers, comment end and start, illegal backslash, and unattached documentation comment) are not affected. Maybe more problematic, the flambda warning 59 is simultaneously not affected and can yield false positive.

Octachron · 2017-09-10T19:11:32Z

typing/typecore.ml

            end;
            let exp =
-              Builtin_attributes.with_warning_attribute pvb_attributes
+              Builtin_attributes.warning_scope pvb_attributes


I would argue that pattern typing should also be done in this warning scope for let binding, in order to be able to write for instance

let[@warning "-8"] [] = []

i.e. update

let (pat_list, new_env, force, unpacks) = type_pattern_list env spatl scope nvs allow in

at line 4012 to take in account the warning attributes on the let binding.

Well spotted! However, note that warning 8 is not processed when type-checking the pattern, but a bit later. I've thus added two warning scopes, and made sure that ppwarning attributes are not processed several times because of that.

Octachron · 2017-09-10T19:16:04Z

typing/typemod.ml

+       let (trem, rem, final_env) = transl_sig (Env.in_signature true env) sg in
+       let rem = simplify_signature rem in
+       let sg = { sig_items = trem; sig_type =  rem; sig_final_env = final_env } in
+       Cmt_format.set_saved_types


Would it not be better to put Cmt_format.set_saved_types outside of the warning scope since it does not emit warning, and even if it would I am not sure that the warning scope should be shared here.

Other calls to Cmt_format.set_saved_types (for Partial_structure_item and Partial_structure) are also under a warning scope (cf end of type_structure, inner run function). I think this harmless: set_saved_types is really just a reference assignment.

Octachron · 2017-09-10T19:18:20Z

typing/typemod.ml

-    md_attributes = pmd.pmd_attributes;
-    md_loc = pmd.pmd_loc;
-  }
+  Builtin_attributes.warning_scope pmd.pmd_attributes


I think it would be better to remove the warning scope here: the only entry point of the approx_* function is the approx_modtype function, and we agreed before that there should be no warnings here.

Thanks, done.

Octachron · 2017-09-10T19:19:18Z

typing/typemod.ml

-                     (extract_sig env smty.pmty_loc mty) in
-          let newenv = Env.add_signature sg env in
-          sg @ approx_sig newenv srem
+          Builtin_attributes.warning_scope sincl.pincl_attributes


Same thing as above: approx functions should probably not concern themselves with warnings .

Thanks, done.

alainfrisch · 2017-09-11T22:21:36Z

One more potential tweak might be the Typetexp.transl_type_param function that does not take in account the attributes on the type parameters. Even, it is currently not possible to emit a warning here, I wonder if it may not be better to enclose it in a warning scope already?

Actually, it is currently impossible to attach attributes to type parameters (it's really a hack to encode them as type expressions in the Parsetree -- and the type checker should report a proper error for unsupported cases instead of a failed assertion).

Still, I've added some code to dead with such attributes, to be more future proof (it would probably be quite useful for ppx to allow attributes there, btw). Note that warnings could be raised by @ppwarning itself (also supported by the warning scope function), as in: type ('a[@ppwarning "XX"], 'b) = 'a -> 'b.

alainfrisch · 2017-09-11T22:27:48Z

Thanks @trefis and @Octachron!

@Octachron : if you are ok with my recent changes following you observations, can you approve the PR so that I can merge it?

Previously, there was a dedicated traversal of the Parsetree to collect and report all ocaml.ppwarning attributes. This approach has the drawback that ocaml.warning settings for the current scope around the ocaml.ppwarnign attribute were not taken into account. Thanks to previous commits, we have a specific place in the code were all attributes go through. So we re-use it to detect ocaml.ppwarning attributes, completely avoiding the dedicated traversal and the drawback mentioned above.

…nctions.

…haustivity.

alainfrisch · 2017-09-11T22:38:56Z

Rebased to trunk.

Octachron

LGTM (once the typing-deprecated test is fixed).

Octachron · 2017-09-12T08:44:36Z

testsuite/tests/typing-deprecated/deprecated.ml

+[%%expect{|
+Line _, characters 26-29:
+Warning 3: deprecated: X.t
+Line _, characters 26-29:


This repeated warning message is no longer here.

alainfrisch added the work-in-progress label Jul 17, 2017

alainfrisch changed the title ~~Clean up and extend support for ocaml.warning/ocaml.warnerror attributes~~ Clean up and extend support for ocaml.warning/ocaml.warnerror/ocaml.ppwarning attributes Jul 18, 2017

alainfrisch removed the work-in-progress label Jul 18, 2017

alainfrisch mentioned this pull request Jul 18, 2017

Avoid "internal" deprecation warnings #1237

Closed

trefis reviewed Jul 18, 2017

View reviewed changes

bobot mentioned this pull request Jul 19, 2017

Per type warn 4 #1071

Open

Octachron reviewed Aug 24, 2017

View reviewed changes

alainfrisch force-pushed the warning_attribute_on_type_exprs branch from 3114320 to 8eb5fc4 Compare August 30, 2017 21:16

alainfrisch mentioned this pull request Aug 30, 2017

Prevent repeated warnings with recursive modules #1191

Merged

trefis approved these changes Sep 10, 2017

View reviewed changes

Octachron reviewed Sep 10, 2017

View reviewed changes

alainfrisch added 3 commits September 12, 2017 00:35

Cleanup processing of warning/warnerror attributes.

e06e0f2

Support ocaml.attribute in type and pattern expressions.

fe08260

Continue adding support for ocaml.warning attribute everywhere.

338552a

alainfrisch added 16 commits September 12, 2017 00:35

Test.

7fa6a8c

Continue + expect test.

895a7f4

Support extension constructors.

e5255c0

Fix doc.

f5c3a4f

Support for attributes on constructors/labels.

9832966

Continue.

8034e26

Documentation.

b074e56

Doc.

478efb5

Delete spurious file.

9a6a7f2

Accept test.

6aeabef

Remove comment. Warnings should be disabled altogether in approx_* fu…

23da3d3

…nctions.

Document that some warnings cannot be controlled by ocaml.warning.

80eeb3a

Use let-binding attributes also for the pattern, and when checking ex…

1d6cbae

…haustivity.

Do not use warning scope in approx_* functions.

fc92111

Be more future proof w.r.t. supporting attributes on type parameters.

893c2bb

alainfrisch force-pushed the warning_attribute_on_type_exprs branch from c717733 to 893c2bb Compare September 11, 2017 22:37

Changelog.

775ead0

alainfrisch added the consider-for-release label Sep 11, 2017

Octachron approved these changes Sep 12, 2017

View reviewed changes

Accept test change.

712dc04

alainfrisch merged commit 52b9a71 into trunk Sep 12, 2017

alainfrisch deleted the warning_attribute_on_type_exprs branch July 2, 2018 06:54

nojb mentioned this pull request Aug 9, 2018

Fix handling of warning attribute attached to type declarations #1977

Merged

This was referenced Mar 14, 2019

Redefinition of exceptions, matching and equality are odd #4531

Closed

Deprecation warnings for types should not be emitted for the definition itself #7005

Closed

ocaml.warning does not work with ocaml.ppwarning #7361

Closed

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

Remove stray mention of domainslib from playground (ocaml#1248)

b2d0f28

Conversation

alainfrisch commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alainfrisch commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trefis commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alainfrisch commented Jul 18, 2017

Uh oh!

alainfrisch commented Jul 18, 2017

Uh oh!

trefis commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alainfrisch commented Jul 18, 2017

Uh oh!

trefis commented Jul 19, 2017

Uh oh!

alainfrisch commented Jul 20, 2017

Uh oh!

nojb commented Jul 20, 2017

Uh oh!

alainfrisch commented Jul 20, 2017

Uh oh!

trefis commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alainfrisch Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alainfrisch commented Aug 30, 2017

Uh oh!

Octachron commented Aug 31, 2017

Uh oh!

alainfrisch commented Sep 6, 2017

Uh oh!

trefis left a comment

Choose a reason for hiding this comment

Uh oh!

Octachron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alainfrisch commented Sep 11, 2017

Uh oh!

alainfrisch commented Sep 11, 2017

Uh oh!

alainfrisch commented Jul 17, 2017 •

edited

Loading

alainfrisch commented Jul 18, 2017 •

edited

Loading

trefis commented Jul 18, 2017 •

edited

Loading

alainfrisch Aug 24, 2017 •

edited

Loading

Octachron Sep 10, 2017 •

edited

Loading

Octachron Sep 10, 2017 •

edited

Loading

alainfrisch commented Sep 11, 2017 •

edited

Loading