Skip to content

Add support for access notes #35665

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Feb 12, 2021
Merged

Conversation

beccadax
Copy link
Contributor

Access notes are sidecar files that can be ingested while a module is being compiled to apply small tweaks to the declarations in the module. They're intended to be used in situations where a module needs to be built with additional internals exposed, but without editing its source code. To make sure the differences from the source code are obvious, the compiler diagnoses each change made by the access notes.

Currently, access notes can:

  • Add or remove @objc attributes
  • Set the selector for an @objc attribute
  • Add or remove dynamic modifiers

In the future, they might gain other capabilities, such as increasing access levels, adding SPI groups, or enabling library evolution for a module.

Fixes rdar://71870054, rdar://71872830.


At the time of this PR's creation, this work is incomplete, but basically functions. I still need to write a more targeted set of tests, but I want to self-review the PR in its current state so I can identify other tasks I have to complete.

evaluator::SideEffect
ApplyAccessNoteRequest::evaluate(Evaluator &evaluator, ValueDecl *VD) const {
AccessNotes &notes = VD->getModuleContext()->getAccessNotes();
if (auto note = notes.lookup(VD))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applying access notes lazily like this is mostly great, except that it becomes impossible to diagnose that an access note doesn't match anything. It might be better to walk through the access notes after parsing, looking up the declarations matching their names and applying them ahead of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs more thought, so I'm adding a FIXME to the test and otherwise leaving it for later.

@beccadax beccadax force-pushed the i-have-some-notes-for-you branch from 66ec5ee to 39abd2f Compare February 9, 2021 02:07
@beccadax beccadax marked this pull request as ready for review February 9, 2021 02:15
@beccadax
Copy link
Contributor Author

beccadax commented Feb 9, 2021

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Feb 9, 2021

Build failed
Swift Test OS X Platform
Git Sha - 39abd2fd37aadf1e39cba0cd86f8c0225a1616c1

beccadax and others added 11 commits February 9, 2021 14:17
Rather than nesting notes for members inside a note for the type, access note files now contain a flat list of notes with parent names embedded in the `Name` property.

Fixes rdar://71872830.

fixup spacing in access notes
An access note with an ObjCName property but no ObjC property now implicitly sets ObjC to true. ObjC: false with an ObjCName is still illegal.
`Name: ‘getter:Context.property()’` now applies an access note to a getter of Context.property, and `Name: ‘setter:Context.property(_:)’` applies one to its setter. Additionally, access notes without `getter:` or `setter:` now cannot apply to accessors.
Access note diagnostics tend to need a fairly specific “diagnose at attr location if possible, decl location otherwise” pattern; encapsulate that in a helper function and use it everywhere.
LLVM’s YAML support wants to show a hard error if an unknown string appears in an access note. Instead, emit a remark but load the parts of the access note we do understand to allow for future expansion of access notes.
• Rename properties for greater consistency with the YAML file
• Rename AccessNotes -> AccessNotesFile
• Add doc comments
• Miscellaneous comment/style improvements.
The LLVM rebranch added an “AllowUnknownKeys” setting to llvm::yaml::Input, which lets us rip out a lot of marginal code and encourages a broader rework of access note error diagnosis:

• Access note warnings and errors are now diagnosed as they’re found during YAML parsing
• They now have proper SourceLocs inside the .accessnotes file
• They’re now tested using -verify-additional-file instead of FileCheck
• A lot of gross duct tape is now gone
@beccadax beccadax force-pushed the i-have-some-notes-for-you branch from 39abd2f to 8d0040f Compare February 10, 2021 08:30
@beccadax
Copy link
Contributor Author

Needed a rebase on top of rebranch, but llvm::yaml has vastly improved in the new LLVM, which made error handling a lot nicer.

@swift-ci please test

@beccadax beccadax changed the title [WIP] Add support for access notes Add support for access notes Feb 10, 2021
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 8d0040f

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8d0040f

@beccadax
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - c250165

@beccadax
Copy link
Contributor Author

The Linux failure was an unrelated transient; the Windows one was literally that they spell an error message with "no" instead of "No".

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - ef97f5c

@beccadax
Copy link
Contributor Author

@swift-ci please test macOS platform

Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and I like the straightforward design.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - ef97f5c

@beccadax
Copy link
Contributor Author

@swift-ci please test macOS platform

@beccadax beccadax merged commit 6c015ca into swiftlang:main Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants