Add library_annotations

[eernstg]

Please add library_annotations to the recommended or core set of lints.

It is currently possible to associate metadata with a library as such by putting it in front of all non-comment/non-whitespace elements in a library. However, this is ambiguous: The metadata might be intended for the library as such, but it might just as well be intended for the first import directive, or whatever comes first after the metadata. Some kinds of metadata could be applicable to both, and it could matter.

If we actively support the habit of putting library metadata on a <libraryName> directive (a plain library; would suffice) then there will be fewer and fewer cases where this ambiguity exists, and we may be able to retire this feature entirely (which means that there must be a <libraryName> for metadata on the library, and the metadata that occurs at the top of the library without a <libraryName> will unambiguously be associated with whatever comes next).

Note that dangling_library_doc_comments is already in core, recommended, and flutter. Both lints have a quick fix.

[parlough]

+1 I'm all for this for being in the core set and eventually dropping support for dangling metadata.

It seems the library_annotations lint is not yet triggering on all cases it could here though. I'm not sure if that's intentional or not, so some investigation and more tests before adding it is probably worthwhile.

[devoncarew]

@natebosch @goderbauer @lrhn - can you review this proposal and give an initial thumbs-up / thumbs-down (with some rational if useful)?

[natebosch]

+1 for putting this lint in the core set. There isn't a good reason to leave this metadata ambiguous when it's very easy to add a library;.

[goderbauer]

+1 to putting this in core.

[lrhn]

+1.

If there is an issue with the implementation, fix that.
Since it's possible to annotate imports, it might just be that the annotation is considered to correctly annotate the next declaration, and shouldn't be moved to a library declaration.

For an annotation with a @Target saying it should go on a library, it's easy. Without such a marker, it's not clear.

[devoncarew]

It seems the library_annotations lint is not yet triggering on all cases it could here though. I'm not sure if that's intentional or not, so some investigation and more tests before adding it is probably worthwhile.

@parlough - do you have examples of where its not triggering (or @pq - do you know of open issue w/ this lint)?

[parlough]

@parlough - do you have examples of where its not triggering

Sorry, just seeing this now.

Taking a look at the implementation, the lint is currently limited to annotations that have used TargetKind in package:meta to declare libraries as their only target. So it won't yet work for dangling @Deprecated, experimental, @JS(), etc. It's not clear to me if there's a particular reason to not trigger for them as well.

[bwilkerson]

It's not clear to me if there's a particular reason to not trigger for them as well.

As Erik pointed out in the first comment, if we don't know that the annotation is intended to be used for the library, then it's ambiguous. It might be intended to apply to the library, or it might be intended to apply to the first directive.

The deprecated annotation is a good example. If the first directive is an export, then we have to assume that the export is what's deprecated, not the whole library. While we might be able to come up with heuristics that would guess correctly most of the time, doing so would still produce more false positives than the current approach, and we decided it wasn't worth the risk.

[devoncarew]

After discussion, we're in favor of adding this lint to the core set. We'd like to avoid ambiguous metadata that's intended for a library directive.

We think that tools which use specific directives (@JS(), ...) should themselves warn if their annotations are attached to unintended things (like the first import statement). It may be worth an audit of similar annotations to make sure they have the intended targetKind.