Register Dependency Arcs for Extension Members #36099
Conversation
|
@swift-ci smoke test |
Fascinating! What happens when one adds a member? The fingerprint changes and the key changes. It might be--have to think about it--as if the old extension were removed and replaced by the new one. It just might work. Or it might be somewhat pessimal. I'm not sure. |
|
This will be much easier to review after the other PR is landed and this one is rebased. We'll need to think about the transitivity. If I change an extension, the arcs will propagate to the nominal and potential member nodes. Then, is there a transitive closure from there? |
|
Now, I'm wondering why these things need names at all. What about a new kind of dependency? |
This was a hole in the existing dependency tracking infrastructure. David managed to discover a way to exploit this bug to get a miscompile in rdar://74583179. There, members of extensions were not counted towards the interface hash of a type and so mutating them could lead to e.g. the wrong declaration being selected in an overload set. To start tracking extensions, we need to add two new kinds of arcs: 1) A nominal arc to the extension 2) A member arc to the extension Unfortunately, extensions are also unique in Swift in that they do not have a name to allow us to unique them. Luckily, we do have a way of identifying extensions: their fingerprint. These arcs are therefore emitted with the extended nominal type and the fingerprint of the extension as their context. This effectively invents a new nominal type for every extension.
CodaFi
force-pushed
the
extensionality
branch
from
cda0f24 to
a99d07a
Compare
|
@swift-ci smoke test |
| return Builder{kind, aspect, cast<NominalTypeDecl>(D), name}; | ||
| const DeclContext *context = dyn_cast<NominalTypeDecl>(D); | ||
| if (!context) { | ||
| context = cast<ExtensionDecl>(D); |
There was a problem hiding this comment.
Not new with this PR, and a nit: When I saw the "cast" I realized that withContext will fail an assert when the D parameter is the wrong kind of Decl. Perhaps the declaration of withContext in the header file would benefit from a comment stating the required types of that parameter?
| .build(); | ||
|
|
||
| // If the subject of this dependency is not the nominal type itself, | ||
| // record another arc for the extension this member came from. |
There was a problem hiding this comment.
This comment needs another sentence or two explaining the "why".
There was a problem hiding this comment.
Also, it ought to explain why just the interface and not the implementation for the sake of future maintainers.
| // Add used members from the perspective of | ||
| // 1) The decl context they are found in | ||
| // 2) The decl context of the request | ||
| // This gets us a dependency not just on `Foo.bar` but on `extension Foo.bar`. |
|
@swift-ci smoke test |
|
@swift-ci please smoke test macos platform |
|
Looks like the test failed for an irrelevant reason. Starting another. |
Built on top of #36096
This was a hole in the existing dependency tracking infrastructure. David managed to discover a way to exploit this bug to get a miscompile in rdar://74583179. There, members of extensions were not counted towards the interface hash of a type and so mutating them could lead to e.g. the wrong declaration being selected in an overload set.
To start tracking extensions, we need to add two new kinds of arcs:
Unfortunately, extensions are also non-nominal in Swift: they do not have a name to allow us to unique them. Luckily, we do have a way of identifying extensions: their fingerprint. These arcs are therefore emitted with the extended nominal type and the fingerprint of the extension as their context. This effectively invents a new nominal type for every extension.