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

Dramatically simplify the UnnecessarilyQualified check #2998

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

carterkozak
Copy link
Contributor

Before this PR

See #2997

Previously we did a rather invasive search on every MemberSelectTree.

After this PR

Now, we first check that the symbol is a type, and the select is operating upon a package, ruling out unnecessary treepath traversals and import scans.

==COMMIT_MSG==
Dramatically simplify the UnnecessarilyQualified error-prone check
==COMMIT_MSG==

@changelog-app
Copy link

changelog-app bot commented Jan 16, 2025

Generate changelog in changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
    and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the
    operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
    performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Dramatically simplify the UnnecessarilyQualified error-prone check

Check the box to generate changelog(s)

  • Generate changelog entry

carterkozak and others added 2 commits January 16, 2025 12:50
Previously we did a rather invasive search on every MemberSelectTree.
Now, we first check that the symbol is a type, and the select is
operating upon a package, ruling out unnecessary treepath
traversals and import scans.
@carterkozak carterkozak force-pushed the ckozak/simplify_UnnecessarilyQualified branch from dd6b88b to 455d47d Compare January 16, 2025 17:50
Comment on lines +67 to +71
"import java.util.Map;",
"import java.util.Map.Entry;",
"import java.util.Set;",
"class Test {",
" Map.Entry<?, ?> get() {",
Copy link
Contributor Author

@carterkozak carterkozak Jan 16, 2025

Choose a reason for hiding this comment

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

Some may prefer for this to end up as Entry rather than Map.Entry, but I'd argue that either one is better than java.util.Map.Entry (and I prefer to list enclosing classes rather than importing them, but that's a stylistic choice).

@bulldozer-bot bulldozer-bot bot merged commit 84854af into develop Jan 16, 2025
5 checks passed
@bulldozer-bot bulldozer-bot bot deleted the ckozak/simplify_UnnecessarilyQualified branch January 16, 2025 18:53
@autorelease3
Copy link

autorelease3 bot commented Jan 16, 2025

Released 6.10.0

@ash211
Copy link
Contributor

ash211 commented Jan 25, 2025

Thank you for this change -- it reduced UnnecessarilyQualified time by over 100x on an internal repo!

Before:

UnnecessarilyQualified                           : 12.412000 seconds (17.41%) 

After:

UnnecessarilyQualified                           : 108 millis       (0.74%)

An unscientific test -- there other changes in the repo than just this change. But it is vastly better now, and I'm going to turn this back on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants