<div dir="ltr"><div dir="ltr"></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 14, 2020 at 12:19 PM Saleem Abdulrasool <<a href="mailto:compnerd@compnerd.org" target="_blank">compnerd@compnerd.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 12, 2020 at 8:50 AM James Y Knight <<a href="mailto:jyknight@google.com" target="_blank">jyknight@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Oct 9, 2020 at 10:21 AM David Rector <<a href="mailto:davrecthreads@gmail.com" target="_blank">davrecthreads@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>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.  <br></div><div><div style="margin:0px;font-stretch:normal;line-height:normal"><br></div><div style="margin:0px;font-stretch:normal;line-height:normal">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.  </div><div style="margin:0px;font-stretch:normal;line-height:normal;min-height:14px"><br></div><div style="margin:0px;font-stretch:normal;line-height:normal">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.</div><div style="margin:0px;font-stretch:normal;line-height:normal;min-height:14px"><br></div><div style="margin:0px;font-stretch:normal;line-height:normal">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.</div><div style="margin:0px;font-stretch:normal;line-height:normal;min-height:14px"><br></div><div style="margin:0px;font-stretch:normal;line-height:normal">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.<br></div></div></blockquote><div><br></div><div>This is an <i>excellent</i> point!</div><div><br></div><div>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 <a href="https://github.com/apple/llvm-project/blob/apple/main/clang/lib/Sema/SemaDeclObjC.cpp" target="_blank">SemaDeclObjC.cpp</a>). 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, "<a href="https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Sema/ExternalSemaSource.h" target="_blank">ExternalSemaSource</a>", which seems like a reasonable place to add a virtual method which may modify newly-created Decls.</div><div><br></div><div>If such a hook mechanism is added, it then seems entirely plausible that <a href="https://github.com/apple/llvm-project/blob/apple/main/clang/lib/Sema/SemaAPINotes.cpp" target="_blank">SemaAPINotes.cpp</a> and <a href="https://github.com/apple/llvm-project/tree/apple/main/clang/lib/APINotes" target="_blank">lib/APINotes/*</a> 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.</div></div></div></blockquote><div><br></div><div>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.</div><div></div></div></div></blockquote><div><br></div><div>Theoretically one could imagine doing so -- but TTBOMK, it's never actually been used like this. All nullability annotations used by objc compilations live in the headers themselves, and the apinotes command-line options are passed only by Swift, for creating a Swift-ObjC-bridge module. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
</blockquote></div></div>