Warning for possibly misspelled built-in attributes

[Octachron]

This pull request adds a simplistic safety net against misspelled built-in attributes.

Since some builtin attributes can be silently deactivated by a spelling error, this patch tries to catch basic spelling mistakes and expose them before they can cause any harms.

The chosen model is a set of opt-in functions for detecting possibly misspelled attribute in parsing/attr_helper using the edit_distance already implemented in util/misc. To decrease the number of false positive the maximal edit distance for identifying misspelling is quite small.

In a semi-related way, this patch also corrects a spelling mistake on the tailcall attribute in the changelog.

[gasche]

I don't understand what context is from this explanation. From the way you use it later I would guess that the words in the context are closed words that we know are not mispellings of the attributes (the implementation seems to somewhat confirm), but nothing in the "context" name or in the explanation here allows me to be sure it works this way.

[Octachron]

Good point. The original meaning was "contextual information useful for reducing the false positive rate". And it currently hold names that might legitimately appear in the same context as the identifier and should not be identified as spelling errors. I will update the explanation and name to better reflect this point.

[alainfrisch]

This is an interesting contribution. One would need some experimenting in the large to see if we get too many false positive. One potential problem is that the new warning implicitly reserves many attribute names; how many of them would be useful for other projects?

One benefit of allowing the form ocaml.XXX is that it makes it easier to spot errors. It could be useful to report any use of an ocaml.XXX warning which is not among those actually known by the compiler. (Similarly, each ppx could report any use of an unknown attribute in "its" namespace.)

[gasche]

One potential problem is that the new warning implicitly reserves many attribute names; how many of them would be useful for other projects?

I was thinking along these lines, but in fact there should be no problem if we assume that -ppx rewriters "upstream" (en amont) remove the occurrences attributes that they are in charge of, instead of just leaving them for the compiler. We should discuss it with @diml (or other people knowledgeable about ppx_driver for example), but if this is the common model (which is only the only one that allows for robust warnings/errors on misspelled attributes) we can assume that the only attributes left for the compiler are meant for it.

[alainfrisch]

there should be no problem if we assume that -ppx rewriters "upstream" (en amont) remove the occurrences attributes that they are in charge of

It's not clear to me that this is necessarily the right model, and this it would completely solve the problem anyway:

• The same attribute might be understood by multiple independent ppx (for instance, a notion of "default value" for a field).
• The compiler and ppx are not the only "clients" of attributes. Tools can react on attributes in .cmi/.cmt files, for instance; or, on the other side, standalone tools that analyze source code.

It would be an interesting project to design a system allowing each package (ppx or other tools) to specify in a declarative way which attributes it understands (and in which context, and perhaps with which payload -- although an error in the payload would typically be detected by the package itself). A dedicated tool (or the compiler itself?) could parse a list of such specifications and apply the corresponding checks. Findlib packages would indicate the spec file so that they can be passed to the checker (or the compiler) easily.

One might not be able to express 100% of contextual constraints (e.g. complex nesting of attributes and/or extension nodes), but this could detect many simple typos, and also provide a good foundation for IDEs/Merling (such as autocompletion on attribute names depending on the context, or "jump to documentation" for a given attribute, assuming the specs contain such information).

[ghost]

With ppx_driver removing attributes that are interpreted by a rewriter is the common model. The only case where the rewriter itself does not remove attributes is type-driven code generators. In this case ppx_type_conv takes care of removing attributes that have been used after all generators have been executed.
So far it's the only case of attribute sharing between rewriters we had. This model could work with ppx_deriving too.

Note that removing attributes has the nice property of making rewriters idempotent.

To deal with attributes that are not used by any rewriters, for instance the ones inserted by merlin, ppx_driver has an option to white-list a namespace. In this case it only checks that these attributes are not dropped by mistake.

Having a system to declare attributes would certainly be nice. Technically we already have that in ppx_core. For us, I don't think it would completely replace the checks done by ppx_core/ppx_driver as they are more strict and have proven to be quite useful so far.

[alainfrisch]

Dropping attributes makes them invisible to tools parsing .cmt/.cmi/.cmti files. For instance, a documentation generator based on .cmti files might want to react on such attributes, even if they have been already intepreted by a ppx (e.g. to show in a compact way the info about automatically generated functions, or to keep e.g. "default" attributes in type declarations).

[ghost]

Currently removing them is the only way for a rewriter to mark them as processed. I guess we wouldn't need to remove them if there was another way, for instance if attributes carried a flag for that.

I'm not sure I understand what you are talking about for documentation, certainly the doc tool couldn't know about all rewriters. It would be more viable to have the ppx rewriters themselves generate specific attributes that the doc tool would interpret, and these could be namespaced.

[alainfrisch]

certainly the doc tool couldn't know about all rewriters

Of course, but it could know about some of them (those from most popular packages), it could be extensible itself to support more attributes, or it could just display generically raw attributes as part of the documentation.

[Octachron]

I agree that false positive and the implit reservation of names could be quite problematic in general. But for the current set of built-in attributes, I do not think it is too problematic: at an edit distance of 1, the valid english words that could be misidentified (at least in the aspell corpus) are

-warning: waning, warnings, earning, darning, warming, warding, warping, warring
-inline: inlined, online, incline
-inlined: inline, unlined, inclined
-unboxed: unbowed
-untagged: (none)

Even for an edit distance of 2, except for "inline" and "warning", the number
of implictely reserved useful names is limited:

-unboxed: unbowed, inboxes, unfixed, unmixed, unbaked, boxed, unbolted, unbound, unyoked
-untagged: untamed, unwaged, untanned, untapped, nagged, tagged, untangled,
snagged, untasted
-inlined: unlined, inland, online, mainlined, lined, inclines, relined, incised,
incited, indited, invited

With few exceptions, these words do not seem to be a problable choice for attribute name.
Moreover, the few exceptions could be added to the list of known false positives.
For instance, the list [unfixed; unmixed; online;line; mainlined; warding; tagged; boxed;unbound] seems quite reasonnable. It could also make sense to adapt the maximal edit distance identifier by identifier to decrease it to 1 for "inline" and "warning".

[bobzhang]

I came across the same issue, how about adding a mutable field processed in the payload, so it will be marked as processed instead of being dropped.

Btw, another item I wish to be mutable is location

[Octachron]

This PR has gone stale and I am not fond of the current implementation; I am therefore closing this PR. Since this seems to be the right kind of target for a compiler plugin, I may reimplement the proposed warning as such at a later date.