Skip to content

[Sema] Raise impact of DefineMemberBasedOnUse to match RemoveInvalidCall #74692

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

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

gregomni
Copy link
Contributor

RemoveInvalidCall has a fix impact of 10, which means that in this issue, the typechecker would rather invent a conformance and then a brand new member rather than find the actual problem.

Raising the impact of DefineMemberBasedOnUse to be equivalent to RemoveInvalidCall seems reasonable, since it is better to diagnose a member that exists but has the wrong type rather than create a member that doesn't exist.

Resolves #74617

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

Please add a test-case from the issue as well.

@@ -11115,7 +11115,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(

auto instanceTy = baseObjTy->getMetatypeInstanceType();

auto impact = 2;
auto impact = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we increase this to 4 and lower RemoveInvalidCall impact to 3, we already increase this by a lot if a the base doesn't have a member.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Testing that.

@gregomni
Copy link
Contributor Author

Yep, just realized that and was adding the test...

@gregomni
Copy link
Contributor Author

Impact scores of 4 and 3 works, and also improves a couple other places that now have better diagnostics. You may want to check the status of radar 86611718 - there's a comment in an improved test mentioning it.

@swift-ci Please smoke test.

@gregomni
Copy link
Contributor Author

Checks didn't run for whatever reason.

@swift-ci Please smoke test.

@gregomni
Copy link
Contributor Author

@swift-ci Please smoke test

@gregomni
Copy link
Contributor Author

@swift-ci Please smoke test.

auto-merge was automatically disabled June 28, 2024 19:32

Pull Request is not mergeable

@gregomni gregomni merged commit 8a40393 into swiftlang:main Jun 28, 2024
3 checks passed
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.

Misleading errors when chaining correct code with incorrect code.
2 participants