Remove internal type from __Sealed attribute in implicit_context.hhi #9626
Conversation
|
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D79505213. (Because this pull request was imported automatically, there will not be any future comments.) |
|
Our HSL CI fails on this diff, I presume because you end up with |
mszabo-wikia
force-pushed
the
remove-nonpublic-type-from-sealed
branch
from
0133deb to
a0a2c97
Compare
|
@mszabo-wikia has updated the pull request. You must reimport the pull request before landing. |
|
@Wilfred Thanks, makes perfect sense. Done. |
D74667088 marked MemoSensitiveImplicitContext as __Sealed, with only a single class permitted to directly extend it. This class is however internal to Meta, causing typechecking errors in OSS Hack. As a fix, mark the attribute with `// @oss-disable` to remove it during the code export process. NOTE: If I'm understanding the docs[1] for `@oss-disable` / `@oss-enable` correctly, this should be the correct way to make this change, which should then transform into `// @oss-disable FBMemoAgnosticImplicitContext::class` once this patch is merged and exported. Let me know if this assumption is incorrect. [1] https://github.com/facebook/buck2/blob/239ab927a5be7dee3035141d29e2e9c91e8ea771/HACKING.md?plain=1#L168
mszabo-wikia
force-pushed
the
remove-nonpublic-type-from-sealed
branch
from
a0a2c97 to
d832512
Compare
|
@mszabo-wikia has updated the pull request. You must reimport the pull request before landing. |
|
Hi there @mszabo-wikia 🙂, Thank you for removing references to these internal names. Making sure these fixes land in upstream is important to all and a right thing to do. I believe there are two small issues with this PR. I have had to remove two references from this commit, but this PR touches only one. hhvm/hphp/hack/hhi/implicit_context.hhi Line 38 in d832512 hhvm/hphp/hack/hhi/implicit_context.hhi Line 52 in d832512 This diff also changed FBMemoSensitiveImplicitContext to FBMemoAgnosticImplicitContext. Is that intentional? |
Unlike in build 25.7.0, this commit removes the attribute outright. This seems to be the direction hhvm OSS is going in: refs facebook#9626
4 participants
D74667088 marked MemoSensitiveImplicitContext as __Sealed, with only a single class permitted to directly extend it. This class is however internal to Meta, causing typechecking errors in OSS Hack.
As a fix, mark the attribute with
// @oss-disableto remove it during the code export process.NOTE: If I'm understanding the docs[1] for
@oss-disable/@oss-enablecorrectly, this should be the correct way to make this change, which should then transform into// @oss-disable FBMemoAgnosticImplicitContext::classonce this patch is merged and exported. Let me know if this assumption is incorrect.[1] https://github.com/facebook/buck2/blob/239ab927a5be7dee3035141d29e2e9c91e8ea771/HACKING.md?plain=1#L168