Fix marking of toplevel attributes in implementations

[nojb]

There seems to be a small regression in 5.2 (probably due to #12451, cc @ccasin): top-level attributes in implementations are not correctly marked as "used".

$ echo '[@@@alert unstable "foo"]' > a.ml
$ ocamlc -c a.ml
File "/home/nojebar/tmp/a.ml", line 1, characters 4-9:
1 | [@@@alert unstable "foo"]
        ^^^^^
Warning 53 [misplaced-attribute]: the "alert" attribute cannot appear in this context

[nojb]

There are some failures in the testsuite that seem wrong; I'll take a look tomorrow.

[shindere]

To be figured out whether this is related to #13155 or not. Hope to be able to provide feedback on this later.

[ccasin]

I think the problem may be not (just) with warning 53, but also with top-level alert attributes themselves. Locally with two files:

a.ml:

[@@@alert unstable "foo"]

let x = 42

b.ml:

let x = A.x

And running:

$ ocamlc -c a.ml
$ ocamlc -c b.ml

I find that on OCaml 4.14.2, I get an alert when compiling the second file, but on OCaml 5.0.0 (and later) I do not. So the warning 53 implementation here seems "correct" with respect to the behavior of the alert attribute in this particular program, but the attribute itself is not behaving as promised in the manual since OCaml 5. I have not investigated what caused this behavior change.

[ccasin]

To be figured out whether this is related to #13155 or not. Hope to be able to provide feedback on this later.

I think this is not related to #13155 - I'm reasonably confident the bug there is just that the driver for the ocaml binary just never calls the function that warns on unused attributes, and I plan to make a fix for that in the next few days.

[shindere]

Awesome! Thanks!

[nojb]

I have not investigated what caused this behavior change.

I think you are missing an -alert flag :)

nojebar@PERVERSESHEAF:~/tmp$ ../ocaml/local/bin/ocamlc -c a.ml
nojebar@PERVERSESHEAF:~/tmp$ ../ocaml/local/bin/ocamlc -c b.ml
nojebar@PERVERSESHEAF:~/tmp$ ../ocaml/local/bin/ocamlc -alert @all -c b.ml
File "b.ml", line 1, characters 8-11:
1 | let x = A.x
            ^^^
Error (alert unstable): module A
foo

[ccasin]

I have not investigated what caused this behavior change.

I think you are missing an -alert flag :)

🤦‍♂️ Indeed.

[nojb]

I pushed a different (and this time, hopefully correect) fix. The key point as I understand it is that floating alerts [@@@alert ...] should only appear at the "top of the file" (this was not being checked). This is now explicitly checked (with a new argument ~top, not to be confused with ~toplevel which is related to whether the evaluation is happening in the REPL). Added a few tests for cases that were not checked before. @ccasin: would you care to do a review of this PR? Thanks!

[ccasin]

@ccasin: would you care to do a review of this PR? Thanks!

Yes - will do.

By the way, I still don't quite understand the alert behavior that confused me above. Using the versions of the compiler in opam, it is the case that to get the alert in my example the -alert +all flag is not needed on 4.14.2 and is needed on 5.0.0. But I don't see any note in the Changes file about a tweak to the behavior of alerts or this flag.

Do you know if this change was intentional? Looking quickly, I don't think the manual says one way or another whether we should expect alerts to be enabled by default. But it seems surprising to me that they are not.

[ccasin]

I think your new test test shows the behavior of w53 is not quite right yet. The attributes in the Sub module are not triggering warning 53, but should.

This becomes clearer if you rename those alerts to be different from the ones at the level of the compilation unit. You will see that line 13 in w53_compunit.ml is triggering the compilation-unit-level alerts, not the ones in Sub. Even a direct reference to W53_compunit_test.Sub.x does not trigger them.

According to the manual, this behavior of the alert attributes is expected (at least, my understanding from the mention of .ml and .mli files in chapter 12.21 suggests these attributes are only expected to work at the compilation unit level - if that's not right, we should update the manual, but testing locally it seems to be).

I can imagine two possible fixes:

1. You mentioned in a previous comment that the toplevel argument to transl_signature was checking whether we were in the REPL - I don't think that's right (but is true about other things in this file with the same name - my fault for picking a confusing name). I think it was correctly checking whether we're at the compilation unit level, the issue is just that it was only doing this for signatures. So we could just add a similar argument to type_structure and that would work. (At least, I think so, I have not tested locally).

But actually I think that will have a different bug. The manual says you can only write compilation-unit-level alert attributes in an ml file if there is no mli. And I think the above will result in failing to issue warning 53 if you write such an attribute and there is an mli. Which maybe is fine for now, but suggests another path:

2. Why are these attributes allowed only if there is no mli? Presumably because if there is an mli we don't look at the ml file to build the cmi. So perhaps these attributes should be marked used as part of building the cmi file, instead - which will happen from the mli file, if present, or the ml file, if not. I'm not actually sure this is feasible, in terms of when we expect to be able to issue warning 53. Happy to explore, but I'm not sure I'll have time in the next few days.

[nojb]

Do you know if this change was intentional? Looking quickly, I don't think the manual says one way or another whether we should expect alerts to be enabled by default. But it seems surprising to me that they are not.

Alerts are enabled by default, except unstable and unsynchronized_access:

ocaml/utils/warnings.ml

Line 868 in 51e4d9a

let default_disabled_alerts = [ "unstable"; "unsynchronized_access" ]

[nojb]

I think your new test test shows the behavior of w53 is not quite right yet.

Thanks for the quick review! You are right, I was also puzzled as to what the exact semantics of alerts across compilation units should be and so the patch is not quite right yet. I will mull over your message but here is a firs set of remarks.

The attributes in the Sub module are not triggering warning 53, but should.

1. You mentioned in a previous comment that the toplevel argument to transl_signature was checking whether we were in the REPL - I don't think that's right (but is true about other things in this file with the same name - my fault for picking a confusing name). I think it was correctly checking whether we're at the compilation unit level, the issue is just that it was only doing this for signatures. So we could just add a similar argument to type_structure and that would work. (At least, I think so, I have not tested locally).

Yes, sorry for the confusion, I was thinking of the ~toplevel argument used in type_structure. But more generally, I am a bit confused: are we supposed to be checking that we are at the compilation unit level, or that we are "at the top of a structure"? I would imagine the latter, if we want floating alerts in submodules to trigger across compilation units.

2. Why are these attributes allowed only if there is no mli? Presumably because if there is an mli we don't look at the ml file to build the cmi. So perhaps these attributes should be marked used as part of building the cmi file, instead - which will happen from the mli file, if present, or the ml file, if not. I'm not actually sure this is feasible, in terms of when we expect to be able to issue warning 53. Happy to explore, but I'm not sure I'll have time in the next few days.

In my mind, not looking at the .ml file if there is an .mli seems reasonable (since this is the way the rest of the language works). In that case I would think that a floating alert in the .ml should trigger the Misplaced_attribute warning, what do you think?

[ccasin]

Yes, sorry for the confusion, I was thinking of the ~toplevel argument used in type_structure. But more generally, I am a bit confused: are we supposed to be checking that we are at the compilation unit level, or that we are "at the top of a structure"? I would imagine the latter, if we want floating alerts in submodules to trigger across compilation units.

I agree this is confusing!

Just testing, it seems that [@@@alert] only works if it is the very first thing in a compilation unit. It would probably be nice if this worked in other places too (submodules), but I do think the fact it does not is expected (it matches what is described in the manual chapter 12.21).

2. Why are these attributes allowed only if there is no mli? Presumably because if there is an mli we don't look at the ml file to build the cmi. So perhaps these attributes should be marked used as part of building the cmi file, instead - which will happen from the mli file, if present, or the ml file, if not. I'm not actually sure this is feasible, in terms of when we expect to be able to issue warning 53. Happy to explore, but I'm not sure I'll have time in the next few days.

In my mind, not looking at the .ml file if there is an .mli seems reasonable (since this is the way the rest of the language works). In that case I would think that a floating alert in the .ml should trigger the Misplaced_attribute warning, what do you think?

I agree: A floating alert should not trigger warning 53 if it is:

• The first signature item in an .mli
• The first structure item in an .ml that is being compiled without an .mli.
  In all other cases, it should.

My comment about cmi files was a suggestion for how to achieve the second bullet: we could mark the warning used as part of constructing the cmi (when compiling ml file, I suppose we only build a cmi if one is absent / we compiled without an mli). But this is probably overcomplicating things and it would also be fine to just explicitly check for the appropriate conditions in typemod.

[nojb]

I pushed a new version of the PR which following some of the suggestions of @ccasin. Rather than trying to propagate enough information to the typechecking routine to be able to mark floating alerts accurately, we delay their marking until after typechecking, when the information is naturally available. Concretely, for both interfaces and implementations, we now:

• mark floating alerts as used unconditionally during typechecking if warning 53 is disabled (this is necessary so that [@@@warning ...] works as expected);
• after typechecking we mark alerts as used while extracting them to store them in the .cmi file; this guarantees that we mark as used exactly those alerts that end up in the .cmi file.

Comments welcome! Thanks.

[ccasin]

Thanks for the changes, I think this came out very well!

It would be nice to have a test showing a top-level alert attribute in an ml will trigger warning 53 if there is an mli, to document this slightly subtle behavior. But I did confirm this works locally, and I don't think the PR needs to be held up for such a test.

Separately, thinking about the interaction of the cmi behavior with various flags made me realize that we'll issue w53 for alert attributes if, for example, the user passes -i or -stop-after parsing. But actually this is already a problem for all attributes, and not made worse by this PR, so again I do not think this PR needs to fix it. I will look at making a PR for that.

[nojb]

Thanks for the changes, I think this came out very well!

Thanks for the review. I pushed an additional test checking that warning is correctly triggered across .cmis both with and without .mlis. Along the way, I noticed a small additional bug: the table of unused attributes was not being reset after doing the reporting, which meant that when compiling multiple source files in a single command-line, we would get duplicated warnings. This is now fixed.

[nojb]

This is ready to go from my side. In order to respect the protocol, this PR would need an approval from another core dev before it can be merged.

[Octachron]

Discarding attributes when they appear in a parsetree location where the warning 53 is disabled sounds useful in general. Maybe we could have a filter function in Buitin_attribute? (and extend its use later on). Moreover, since the new code is not dependent on the toplevel argument, would it make sense to move it to the warning_attribute function ?

[nojb]

Good idea bout merging this logic inside warning_attribute; done in 73c8720. This fixes yet another small bug: in the following program,

class c =
  object
    [@@@warning "-53"]
    [@@@alert foo "foo"]
  end

the alert would trigger Warning 53 even though it the warning is supposed to be disabled. After moving the logic to warning_attribute, the warning is no longer triggered.

I added a few more tests to exhibit this behaviour.

[Octachron]

Overall, I agree that this is a nice if subtle fix. Moreover, the fix makes the correctness of the code easier to check by moving the code marking the alerts closer to the consumer of the alerts which is Env.save_signature.

My remarks are minor and can be safely ignored (or delayed to a later PR).

[nojb]

Thanks for the review @Octachron! Am planning to merge the the PR once the CI passes.