Rework warning 53 (misplaced attributes) to work on all attributes in all places #12451

ccasin · 2023-08-01T20:48:11Z

This PR reworks warning 53 (misplaced attribute) so that it works for all attributes and is issued in a uniform way.

Presently, there are many attributes known to the compiler for which this warning is never issued (even though there are many places one can put them that they will be ignored). Those for which the warning can be issued are handled by just looking for them in specific places we think someone might mistakenly write them. The goal of this PR is to fix that situation.

The main idea is to take an approach like ppxlib: When the parser sees an attribute, it adds it to a table. When the compiler uses the attribute, it marks it as used by removing it from the table. When compilation is done, we check whether any attributes remain in the table, and issue warnings for them.

We only track known compiler built-in attributes for the warning. There is now a list of such attributes in Builtin_attributes. While most ppxes now remove their attributes after processing them, not all do, and other attributes may be in the program for various reasons. To deal with the problem that the compiler can't know whether non-built-in attributes are misplaced, we only track those attributes which appear in the list of built-ins.

When we deployed this within Jane Street, we found more than 200 misplaced attributes in our code base. And this PR even cleans up a few misplaced attributes in the stdlib. So, I believe this is a useful addition.

Reviewing

This PR isn't massive, but I've broken it into several commits to create an opportunity to explain a few subtleties:

Commit 1: Add attr tracking mechanism, but don't whitelist any attrs yet

This adds all the new attribute tracking code, but leaves the list of tracked attributes empty and the old w53 stuff in place. So all the code to add attributes to the table is there and fires, but no attributes are on the whitelist so they aren't added.

A couple notes about when attributes are added to the table:

When to add attributes: It's not sufficient to just add attributes when they are parsed in the parser, because not all parsetrees come from the parser. The -pp flag will read a marshalled parsetree instead, and ppxes may be used to modify the parsetree.

We want to track only attributes in the final version of the parsetree. We can accomplish this by adding attributes to the table in two cases:

If no -ppx or -pp flags are passed, the parser will be run and the parsetree won't be modified, so we add attributes during parsing.
If either of those flags is passed, the Ast_invariants check will be run on the final parsetree, so we add attributes during that check instead.

Attributes in attributes: Attribute payloads can contain more attributes. This happens in practice, if rarely.

These inner attributes should not be tracked for w53 - we can't know the meaning of the outer attribute, so we have no way to know what to do with them and whether or not they were used appropriately.

If we're adding attributes during the Ast_invariants check, that's easy - just update the iterator when descending into an attribute payload. In the parser, it's harder. I've done the dumb thing of just removing any nested attributes from the table after parsing an attribute. This wastes a little effort, but attribute payloads are typically small and nested attributes are rare, so it shouldn't be too bad. This strategy seemed much simpler and easier to review than trying to thread some state through the parser, but I'm open to other suggestions.

Commit 2: Rework w53 tests

This replaces the w53 test with a new one that tests all compiler built-in attributes. For each attribute, I tried to include examples of where it is allowed to appear and where it is not and w53 is issued (if such a place exists). The corresponding reference file is updated over the next two commits as attributes are added to the whitelist.

Commit 3: Handle the basic attributes

This adds most attributes for the whitelist, leaving off those related to warnings (which need to be handled a little differently).

The bulk of the diff here is in lambda/translattribute.ml. There are several related changes:

We should use the accessors in Builtin_attributes to look for specific attributes, ensuring they are marked as used.
A few functions, like add_local_attribute, are slightly rearranged so that attributes aren't checked for until we're sure we're in a context where they can be used (since the act of looking for the attribute marks it as used).
It's no longer necessary to remove these attributes from the terms when they are used. The old implementation of w53 for those attributes, which relied on checking whether attributes remained on terms after translation, is removed.
Care must be taken to mark an attribute as used even if we're compiling with a configuration that does not use it. E.g., the unrolled attribute only has an effect on the program if compiling with flambda, but should not issue w53 when compiling with closure.

This commit fixes a bug in find_attribute, I think: It used to be that if an attribute was written twice on the same term (e.g., [@inline] [@inline]), it was removed with a warning. Now one of them is used, with a warning.

It also removes a few misplaced [@inline] attributes in the stdlib (these attributes do nothing in mli files).

Commit 5: Handle attributes like "warning" and "alert"

Attributes like [@alert] are "used" by placing them in the environment, where they may be noticed later. So this commit adds code to env.ml to mark such attributes used when they are added to the environment as part of some form that is allowed to use them. The special case of a file-level [@@@alert] is handled in typemod.ml.

In the stdlib there is an [@@@alert] attribute in the hashtable template file which is particularly annoying: That template is included in one place at the top level of a file (so the attribute is legal) and another place inside a module (where the attribute has no effect). I've dealt with this by using warning attributes to disable w53 here.

Commit 6: `[@ppwarning]` with a bad payload should give w47, not w53

This just fixes a minor issue I noticed in the final state of this PR. [@ppwarning] with a bad payload used to be silently ignored, but after this PR it would issue w53. But warning 47 (bad attribute payload) is more appropriate.

ccasin · 2023-08-01T21:02:30Z

There is a problem here with the stdlib template mechanism (hashtbl.template.mli has an attribute that works fine in hashtbl.mli but is incorrect when that signature is inlined in moreLabels.mli, because [@@@alert] only works at the file level).

~~I don't know much about the stdlib template mechanism, so I'm not immediately sure what the right fix is. Happy for suggestions, or I'll look closer tomorrow.~~

(Now fixed, see below)

ccasin · 2023-08-05T17:31:17Z

OK, I've (hopefully) fixed the stdlib / templates issue by making warning 53 for top-level alerts controllable with warning attributes, and turning it off around the relevant attribute in the template file. It would be nice to make it fully controllable with warning attributes, but that seems a bit tricky and best left for a subsequent PR. Though I'm open to other suggestions regarding the template file issue.

I've rebased, adjusted the history, and force pushed, putting this fix as part of the commit that dealt with alert attributes and cleaning the bootstrapping into its own commits. If anyone wants to see just the fix added for the template issue, I put it here. I've adjusted the original PR description to reflect this.

Octachron

I have some minor remarks on the Builtin_attributes interface with respect to the ocaml namespace for attributes, but otherwise that PRs feels like a clear improvement to me.

Marking built-in attributes as we use them should be far less error prone than trying to remember to check if some builtin attributes were left unused somewhere or somewhen.

parsing/builtin_attributes.ml

Octachron · 2023-10-24T12:33:20Z

parsing/builtin_attributes.ml

+  ; "warnerror"; "ocaml.warnerror"
+  ; "warning"; "ocaml.warning"
+  ; "warn_on_literal_pattern"; "ocaml.warn_on_literal_pattern"
+  ]


Rather than listing each built-in in its prefixed and non-prefixed form, I propose to move the prefixing logic to the lookup functions.

I've made this change, and the similar suggested changes below. This resulted in some additional simplifications (various functions that used to take lists can now just take a single attribute name).

I was on the fence, after reading the suggestion, because a consequence of this move is that we'll needlessly allocate additional strings when we strip ocaml. prefixes. However, after seeing the simplifications, I'm happy with the change. (Also, for small programs or programs with no attributes, this is probably just a win.)

Octachron · 2023-10-24T12:35:23Z

parsing/builtin_attributes.ml

@@ -67,6 +133,40 @@ let error_of_extension ext =
  | ({txt; loc}, _) ->
      Location.errorf ~loc "Uninterpreted extension '%s'." txt

+let mark_alert_used a =
+  match a.attr_name.txt with
+  | "ocaml.deprecated"|"deprecated"|"ocaml.alert"|"alert" ->


Similarly, do we want to add an unprefix function that removes the ocaml. prefix to avoid the duplication here?

Octachron · 2023-10-24T12:37:48Z

parsing/builtin_attributes.ml

@@ -243,34 +351,36 @@ let warning_scope ?ppwarning attrs f =
    Warnings.restore prev;
    raise exn

-
-let warn_on_literal_pattern =
+let has_attribute nms attrs =


As far as I can see, all arguments of has_attribute have the form ["ocaml." ^ attribute, attribute] which suggest to move the namespacing logic inside the has_attribute function.

parsing/builtin_attributes.mli

parsing/builtin_attributes.ml

parsing/builtin_attributes.mli

parsing/builtin_attributes.ml

parsing/builtin_attributes.mli

ccasin · 2023-10-29T17:48:27Z

@Octachron Thanks very much for the thoughtful review, and apologies for the delay in getting back to this. I've responded to a couple points above. I meant to rebase and integrate the review feedback today as well, but got distracted by tracking down #12697 and am now out of time. I will return later in the week to integrate your suggestions.

The reference file is not correct yet - it will be updated in subsequent commits as we track attributes with the new mechanism.

Needs a bootstrap due to removing misplaced attrs in the stdlib. That's done in next commit.

This also makes w53 controllable with warning attributes, but only for top-level alerts. This is used to handle the fact that in the stdlib there's a template file with a top-level alert attribute, and it's included in two different places, one where that attribute is legal, and one where it is not A boostrap is needed, and performed in the next commit.

This also simplifies the types of several functions, which no longer need to take lists.

ccasin · 2023-10-31T19:28:09Z

@Octachron I believe I have addressed all your comments above with four new commits, and this is ready for further review. I've also rebased it on to the latest trunk. Thanks again!

Octachron · 2023-11-09T09:20:38Z

parsing/builtin_attributes.ml

+  if String.starts_with ~prefix:"ocaml." s && len > 6 then
+    String.sub s 6 (len - 6)
+  else
+    s


Note that it seems that in a handful of cases, we could avoid the allocation by having a attr_equal function rather than using the String.equal (drop_ocaml_attr_prefix x) y pattern.

Good idea - I've made this change, leaving only the allocation in is_builtin_attr. This seems like a fine compromise.

Octachron · 2023-11-09T09:39:53Z

testsuite/tests/warnings/w53.ml

+end
+
+module TestDeprecatedStruct = struct  (* CJC XXX THIS IS ALL BUGGY *)
+  let x = 5 [@deprecated] (* rejected *)


What is buggy in this test?

Oops, sorry, nice catch! This was a stray comment from an intermediate state, now deleted.

Octachron

The PR looks good to merge to me (except for some minor remarks).
I really like the fact that with this PR the misplaced attribute warning will only require care from attribute authors.

ccasin · 2023-11-09T17:05:47Z

I've integrated the last round of minor feedback, and believe this is good to go when the tests pass.

gasche · 2023-11-09T21:54:14Z

What should we do with the git history? Squash everything, something else?

ccasin · 2023-11-09T22:45:39Z

What should we do with the git history? Squash everything, something else?

I think squashing is reasonable - it should read fine as one commit. But I'm open to suggestions if you'd like me to reorganize it.

Octachron · 2023-11-10T12:40:36Z

The commit history seems mostly linear (no back-and-forth) and contains some bootstrap, thus I think it is clearer to merge the PR as it is.

ccasin force-pushed the warning-53 branch 2 times, most recently from 1593c39 to 4d037f1 Compare August 5, 2023 17:30

Octachron self-requested a review September 20, 2023 13:10

Octachron self-assigned this Sep 20, 2023

Octachron reviewed Oct 24, 2023

View reviewed changes

ccasin mentioned this pull request Oct 29, 2023

Errors for marshalled ASTs no longer print relevant source code #12697

Closed

ccasin force-pushed the warning-53 branch from 455e3d0 to b27fa62 Compare October 31, 2023 17:51

ccasin and others added 12 commits October 31, 2023 13:52

Add attr tracking mechanism, but don't whitelist any attrs yet

35501bd

Rework w53 tests.

64d6327

The reference file is not correct yet - it will be updated in subsequent commits as we track attributes with the new mechanism.

Handle the basic attributes.

2daacd9

Needs a bootstrap due to removing misplaced attrs in the stdlib. That's done in next commit.

bootstrap, promote parser, make depend

00893de

bootstrap

4659b7c

[@ppwarning] with a bad payload should give w47, not w53

20e8696

changes entry

2086c08

integrate review: move namespace logic to eliminate duplication

968b5f7

This also simplifies the types of several functions, which no longer need to take lists.

integrate review: rename [attr_tracking_time] and [filter_attributes]

3bc54da

integrate review: doc sections in builtin_attributes

2bf1701

integrate review: missing attributes

e74de27

ccasin force-pushed the warning-53 branch from b27fa62 to e74de27 Compare October 31, 2023 19:13

Octachron reviewed Nov 9, 2023

View reviewed changes

Octachron approved these changes Nov 9, 2023

View reviewed changes

integrate review: allocate fewer strings and delete a stray comment

e623fd7

Octachron merged commit 718553e into ocaml:trunk Nov 10, 2023
9 checks passed

lthls mentioned this pull request Feb 12, 2024

Regression around warning 53 (misplaced-attribute) and [@@ocaml.inline always]. #12972

Closed

anmonteiro mentioned this pull request Mar 18, 2024

feat: upgrade core to OCaml 5.2 melange-re/melange#1074

Merged

jonludlam mentioned this pull request Mar 27, 2024

Misplaced attributes for deprecation markers #13054

Open

anmonteiro mentioned this pull request Mar 30, 2024

fix: forward-compatible fix for OCaml 5.2 ocaml/dune#10342

Merged

Octachron mentioned this pull request Apr 12, 2024

manual: document poll built-in attribute #13092

Merged

voodoos mentioned this pull request May 7, 2024

[5.2] Build occurrences index for Merlin ocaml/dune#10422

Merged

Octachron mentioned this pull request May 9, 2024

Missing warning when an attribute is ignored or makes no sense #13155

Closed

nojb mentioned this pull request May 15, 2024

Fix marking of toplevel attributes in implementations #13170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework warning 53 (misplaced attributes) to work on all attributes in all places #12451

Rework warning 53 (misplaced attributes) to work on all attributes in all places #12451

ccasin commented Aug 1, 2023 •

edited

ccasin commented Aug 1, 2023 •

edited

ccasin commented Aug 5, 2023 •

edited

Octachron left a comment

Octachron Oct 24, 2023

ccasin Oct 31, 2023

Octachron Oct 24, 2023

Octachron Oct 24, 2023

ccasin commented Oct 29, 2023

ccasin commented Oct 31, 2023 •

edited

Octachron Nov 9, 2023

ccasin Nov 9, 2023

Octachron Nov 9, 2023

ccasin Nov 9, 2023

Octachron left a comment

ccasin commented Nov 9, 2023

gasche commented Nov 9, 2023

ccasin commented Nov 9, 2023

Octachron commented Nov 10, 2023

Rework warning 53 (misplaced attributes) to work on all attributes in all places #12451

Rework warning 53 (misplaced attributes) to work on all attributes in all places #12451

Conversation

ccasin commented Aug 1, 2023 • edited

Reviewing

Commit 1: Add attr tracking mechanism, but don't whitelist any attrs yet

Commit 2: Rework w53 tests

Commit 3: Handle the basic attributes

Commit 5: Handle attributes like "warning" and "alert"

Commit 6: [@ppwarning] with a bad payload should give w47, not w53

ccasin commented Aug 1, 2023 • edited

ccasin commented Aug 5, 2023 • edited

Octachron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccasin commented Oct 29, 2023

ccasin commented Oct 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Octachron left a comment

Choose a reason for hiding this comment

ccasin commented Nov 9, 2023

gasche commented Nov 9, 2023

ccasin commented Nov 9, 2023

Octachron commented Nov 10, 2023

ccasin commented Aug 1, 2023 •

edited

Commit 6: `[@ppwarning]` with a bad payload should give w47, not w53

ccasin commented Aug 1, 2023 •

edited

ccasin commented Aug 5, 2023 •

edited

ccasin commented Oct 31, 2023 •

edited