[Sema] Score non-settable overload choices higher in RValueToLValue fix #74243
Conversation
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
xedin
reviewed
Jun 10, 2024
lib/Sema/CSSimplify.cpp
Outdated
| if (auto decl = overload->choice.getDeclOrNull()) { | ||
| if (auto VD = dyn_cast<VarDecl>(decl)) { | ||
| if (!VD->isSettableInSwift(DC)) { | ||
| impact += 10; |
There was a problem hiding this comment.
I think this might be too dramatic of an increase, we just need this to be slightly higher then default impact, increase it by too much and it would start competing with other solutions just like what you have in tests where count is a method and a property.
There was a problem hiding this comment.
Yep! I had gotten the mistaken impression that the impact was local to the specific choice location. Changed to just += 1.
gregomni
force-pushed
the
rvalue-ambiguous
branch
2 times, most recently
from
ef7300a to
909553f
Compare
|
@swift-ci Please smoke test. |
xedin
approved these changes
Jun 14, 2024
lib/Sema/CSSimplify.cpp
Outdated
|
|
||
| // An overload choice that isn't settable is least interesting for diagnosis. | ||
| if (auto overload = findSelectedOverloadFor(getCalleeLocator(fix->getLocator()))) { | ||
| if (auto decl = overload->choice.getDeclOrNull()) { |
There was a problem hiding this comment.
All of the nested conditionals could be simplified down to:
if (auto *var = dyn_cast_or_null<VarDecl>(overload->choice.getDeclOrNull())) {
impact += !var->isSettableInSwift(DC) ? 1 : 0;
}…agnoses from better choices.
gregomni
force-pushed
the
rvalue-ambiguous
branch
from
909553f to
843ce58
Compare
|
@swift-ci Please smoke test. |
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.
With multiple overload choices, TreatRValueAsLValue used to just pick the first solution. We get better diagnostics if we pick a solution where the chosen VarDecl is settable. So make the impact of fixing non-settable overload choices higher, so we get a settable overload, if one exists.
I.e. if you conform to two protocols with same-named property, one of which is {get} and the other is {get set} and both possibilities end up with the same fix in the same location, it isn't much help to use the "it's a get-only property" choice.
Resolves #46265
Removed the previous commit and force pushed my branch before committing the new changes, and hadn't realized that makes GitHub automatically close the PR when there's (briefly) no changes. Oops. So here's a new PR.