[cfe-dev] Adding Support for APINotes

[Saleem Abdulrasool via cfe-dev]

On Mon, Oct 12, 2020 at 8:50 AM James Y Knight <jyknight at google.com> wrote:

>
>
> On Fri, Oct 9, 2020 at 10:21 AM David Rector <davrecthreads at gmail.com>
> wrote:
>
>> IIUC APINotes wants to add attributes during parsing so that the those
>> attributes are then treated just like real parsed attributes would be by
>> Sema.
>>
>> Ideally this sort of thing should be able to be contained in an
>> ASTConsumer, but it presently cannot: the only real option ASTConsumer
>> provides is to traverse the AST post-parsing in an override of
>> `HandleTranslationUnit()`.  This leaves APINotes no choice but to add its
>> details directly to Sema and Parser, with the result that Swift-specific
>> details would necessarily show up in the guts of mainline clang were this
>> to be upstreamed.
>>
>> If ASTConsumer instead provided a comprehensive virtual interface (but
>> with negligible overhead, somehow) to perform custom consumer actions at
>> strategic points during parsing/Sema (e.g., call consumer virtuals in each
>> Sema::ActOn*), this conflict probably could be, or at least could have
>> been, avoided.
>>
>> Note too that this issue resonates with one a user faced just the other
>> day: he wanted to call `Sema::LookupName` within an ASTMatchers callback
>> (during HandleTranslationUnit, as usual) to determine if a certain node
>> could be rewritten w/o changing its semantics — but of course the necessary
>> Sema/Parser scope info is long gone by that point.  If only he had the
>> option to perform his logic on the node immediately after it was parsed,
>> there would be no issue. Because he cannot, his only option was to
>> laboriously recreate that state data in order to call LookupName.
>>
>> In sum, the dearth of ASTConsumer virtuals hinder encapsulation, causing
>> 1) Sema and Parser to be polluted with what should be a consumer’s details,
>> and 2) consumers to be polluted with what should be Sema & Parser details,
>> and making it difficult to upstream useful tools.
>>
>
> This is an *excellent* point!
>
> It looks like the primary change in Clang which is required for APINotes
> is to place "ProcessAPINotes" calls into each of the Sema::ActOn* functions
> which creates a (subclass of) Decl. This needs to occur after creating the
> Decl object and processing the attributes in the source, but prior to
> actually acting on it (e.g. see apple's SemaDeclObjC.cpp
> <https://github.com/apple/llvm-project/blob/apple/main/clang/lib/Sema/SemaDeclObjC.cpp>).
> This call could instead be a call to a generic hook for modifying the decl,
> rather than specifically for processing API notes. We already even have a
> container for such hooks in Sema, "ExternalSemaSource
> <https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Sema/ExternalSemaSource.h>",
> which seems like a reasonable place to add a virtual method which may
> modify newly-created Decls.
>
> If such a hook mechanism is added, it then seems entirely plausible that
> SemaAPINotes.cpp
> <https://github.com/apple/llvm-project/blob/apple/main/clang/lib/Sema/SemaAPINotes.cpp> and
> lib/APINotes/*
> <https://github.com/apple/llvm-project/tree/apple/main/clang/lib/APINotes> can
> be moved from the clang-fork into swift itself without requiring
> significant modifications of the code, or any change in functionality of
> the feature.
>

Well, the APINotes also provide nullability information for ObjC
interfaces, which is not part of Swift, but rather part of ObjC, so that
still needs to be in clang.  That is, the functionality is useful for ObjC,
and that doesn't make sense to be in Swift.


> If this works out how it seems like it could, this seems like it may even
> be a better outcome for all parties (both the potential static analysis
> tooling use-cases, and for Swift).
>


-- 
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20201014/56c209f6/attachment.html>