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

[QoI, Crasher] Don't include type variables in missing generic requirement match info #31869

Merged
merged 2 commits into from May 20, 2020

Conversation

gregomni
Copy link
Collaborator

If the missing generic requirement includes type variables, don't emit an unhelpful note for it.

Resolves SR-12759

@gregomni
Copy link
Collaborator Author

@swift-ci Please smoke test.

@gregomni gregomni requested a review from CodaFi May 18, 2020 20:48
@@ -784,6 +784,9 @@ static Optional<RequirementMatch> findMissingGenericRequirementForSolutionFix(
default:
return Optional<RequirementMatch>();
}

if (missingType->hasTypeVariable())
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to actually provide a solution here and use solution.simplifyType on missingType? That should make sure that there are no type variables.

Copy link
Member

Choose a reason for hiding this comment

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

Solution associated with the fix considered that is.

Copy link
Member

Choose a reason for hiding this comment

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

Will a damaged solution always substitute out every type variable?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if there was a fixed solution produced it would not leave any type variables after substitution.

Copy link
Member

Choose a reason for hiding this comment

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

Fantastic. Then it seems like we just have to substitute in each one of these little closures that act as shared return points and assert that we're not leaking type variables.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that would make sense!

@CodaFi
Copy link
Member

CodaFi commented May 18, 2020

Note that, as written, this cuts off emitting these diagnostics altogether for a lot of common generic cases. These diagnostics are really valuable, and I don't think we should consider this solved until we can emit them again.

Copy link
Member

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

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

As noted, we should try to simplify all types transiting out of this function against the (broken) solution then assert that they no longer contain type variables.

…e taus to generic param names for emissions.
@gregomni
Copy link
Collaborator Author

Makes sense!

@swift-ci Please smoke test.

@gregomni gregomni requested a review from CodaFi May 19, 2020 01:54
Copy link
Member

@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.

Makes sense ditto mapping out of context which, it seems, should have a better solution...

@CodaFi
Copy link
Member

CodaFi commented May 19, 2020

@swift-ci ASAN test

@CodaFi
Copy link
Member

CodaFi commented May 20, 2020

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.

None yet

3 participants