hhvm icon indicating copy to clipboard operation
hhvm copied to clipboard

Remove internal type from __Sealed attribute in implicit_context.hhi

Open mszabo-wikia opened this issue 5 months ago • 10 comments

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 avatar Aug 02 '25 13:08 mszabo-wikia

@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.)

facebook-github-bot avatar Aug 02 '25 13:08 facebook-github-bot

Our HSL CI fails on this diff, I presume because you end up with <<__Sealed()>> which doesn't allow extension at all. Perhaps we should just not seal it in OSS?

Wilfred avatar Aug 05 '25 12:08 Wilfred

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot avatar Aug 05 '25 13:08 facebook-github-bot

@Wilfred Thanks, makes perfect sense. Done.

mszabo-wikia avatar Aug 05 '25 13:08 mszabo-wikia

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot avatar Oct 06 '25 11:10 facebook-github-bot

Hi there @mszabo-wikia :slightly_smiling_face:,

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.

https://github.com/facebook/hhvm/blob/d832512151f8a72759b400e1e86941f0312e5022/hphp/hack/hhi/implicit_context.hhi#L38

https://github.com/facebook/hhvm/blob/d832512151f8a72759b400e1e86941f0312e5022/hphp/hack/hhi/implicit_context.hhi#L52

This diff also changed FBMemoSensitiveImplicitContext to FBMemoAgnosticImplicitContext. Is that intentional?

hershel-theodore-layton avatar Nov 02 '25 10:11 hershel-theodore-layton

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot avatar Nov 03 '25 11:11 facebook-github-bot

@hershel-theodore-layton Thanks, yeah that was meant to go on the other class. Fixed now.

mszabo-wikia avatar Nov 03 '25 11:11 mszabo-wikia

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot avatar Dec 03 '25 22:12 facebook-github-bot

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot avatar Dec 10 '25 11:12 facebook-github-bot

@vassilmladenov 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.)

meta-codesync[bot] avatar Dec 10 '25 17:12 meta-codesync[bot]

@vassilmladenov merged this pull request in facebook/hhvm@a455f079d5bc324514bc5740d0fafbd5aef5af9b.

meta-codesync[bot] avatar Dec 11 '25 11:12 meta-codesync[bot]