Skip to content
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

Externs inner namespaces fix #4069

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Externs inner namespaces fix #4069

wants to merge 1 commit into from

Conversation

0c3d7r4
Copy link

@0c3d7r4 0c3d7r4 commented Mar 14, 2023

Inner namespace consolidation.

📝 The write up: The Bitter Taste Of Bazel In My Morning Java, Or How I Consolidated Google Namespaces .

I understand you might not go with the isFromEmptyObjLitExtern as in the PR and choose to properly fix if (isObjectLiteralThatCanBeSkipped(type)) instead, i.e., by maintaining droppedPropertiesOfUnions correctly. It's all in the blog post.

Please leave a comment to acknowledge the presence of the bug due the making of the recursive function call come first 4f69363 which results in types being substituted to the sentinel one without recording their dropped properties.

@google-cla
Copy link

google-cla bot commented Mar 14, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@niloc132
Copy link
Contributor

Maybe add tests for this?

We've personally run into this especially when trying to patch closure's own externs from a downstream project - there are lots of cases where merging not only namespaces, but also fully unioning externs, which we've had trouble with (but maybe it has gotten better).

Finally, in lieu of a comments section in the blog, bazel itself advertises a plugin for IntelliJ, which is not terrible, though does have some pretty substantial warts (for example, it effectively never works with a recent IJ build, and you sometimes need to exclude some targets from being indexed). Once installed and configured, as long as you're comfortable with the bazel command line, it is pretty decent though.

@lauraharker
Copy link
Contributor

Thanks doing all this investigation. I'll take a more thorough look after the weekend, but a few early comments:

  1. as niloc132 suggested, could you add unit tests? That would help me review this change (and prevent future regressions of the sort you found in r20220104 😀 )
  2. please attempt to sign the Google CLA and let me know if you have any problems. import this PR to google's internal repo for testing is blocked on a valid CLA.
  3. I'll look closer at what's going on with droppedPropertiesOfUnions since as you suggest in the PR summary, I find having this very specific PrototypeObjectType method & also the fact that it's only used in the missing property checks to be suspicious.

@lauraharker
Copy link
Contributor

Ok, I definitely believe the right fix is to unify the behavior of isObjectLiteralThatCanBeSkipped and the code adding to droppedPropertiesOfUnions, so that we either stop skipping literal objects or start tracking their dropped properties. I'd expect that just tracking them in droppedPropertiesInUnions would be better for performance and bloat the size of the type registry.

Are you interested in making that change in this PR? Sounds great if you are, otherwise please hand this off via filing an issue.

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