UnnecessaryExplicitTypeArguments: retain load-bearing witness in argument position on instance methods too#946
Merged
Conversation
…ment position on instance methods too The argument-position branch only guarded static methods (via shouldRetainOnStaticMethod) against removing a witness whose type variables aren't inferable from the call's own arguments. Instance methods used as arguments skipped that guard entirely, so a witness like `t.<STATE_VALUE>source()` passed to a generic static factory `Transition.of(...)` got stripped even though STATE_VALUE has no argument to infer it from and the factory itself provides no target type, leaving STATE_VALUE unresolved. Generalize the check to apply regardless of static/instance, matching the reasoning already used for the sibling select-position case.
neil-mushell
force-pushed
the
neil-mushell/fix-unnecessary-explicit-type-arguments-argument-position
branch
from
July 21, 2026 14:41
5977e5c to
03032b8
Compare
timtebeek
approved these changes
Jul 21, 2026
timtebeek
left a comment
Member
There was a problem hiding this comment.
Thanks for coming back here with another round of fixes. Good to have this hardened against a code base with heavy usage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's changed?
Generalizes
UnnecessaryExplicitTypeArguments's argument-position guard to retain a type witness on instance methods too, not just static ones, when the witness's type variables aren't inferable from the call's own arguments. Removes the now-redundantshouldRetainOnStaticMethodhelper in favor of applyingcanInferTypeArgumentsFromArgumentsuniformly — the same check already used for the sibling select-position branch.What's your motivation?
The argument-position branch only guarded static methods (via
shouldRetainOnStaticMethod) against removing a witness whose type variables aren't inferable from the call's own arguments. Instance methods used as arguments skipped that guard entirely, so a witness liket.<STATE_VALUE>source()passed to a generic static factoryTransition.of(...)got stripped even thoughSTATE_VALUEhas no argument to infer it from and the factory itself provides no target type, leavingSTATE_VALUEunresolved.We hit this piloting the recipe against a state-machine interpreter with recursive/self-bounded generics (
StateMachineInterpreter<STATE_VALUE extends Enum<STATE_VALUE>, ...>), where it silently broke compilation.Anything in particular you'd like reviewers to focus on?
Whether folding the argument-position and select-position branches into one shared check is worth a larger refactor — they both now reduce to "retain unless inferable from the call's own arguments" and only diverge afterward on the overload-ambiguity check, which is argument-position-only.
Anyone you would like to review specifically?
No specific request — open to whoever's around.
Have you considered any alternatives or workarounds?
Considered keeping
shouldRetainOnStaticMethod's name/shape and just widening its condition (adding|| !isStaticor similar). Removed it instead since "static-only" naming became misleading once the check applies to both static and instance methods, and both branches already sharecanInferTypeArgumentsFromArguments.Any additional context
Full module suite: 2147 tests, 0 regressions from this change (one pre-existing intermittent failure in
AllBranchesIdenticalTest.collapseIdenticalBranchesPython, reproduced independent of this diff — a flaky Python-parser round-trip test unrelated to this recipe). Added both a negative-case test (witness retained, matching the bug) and a positive-case test (witness still removed when inference from the call's own arguments is possible) to make sure the fix doesn't over-correct into blanket retention.Checklist