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

[Sema] When resolving type declarations, if there is an error with a typealias,... #19830

Merged
merged 6 commits into from Oct 14, 2018

Conversation

gregomni
Copy link
Collaborator

@gregomni gregomni commented Oct 11, 2018

... check to see if the type alias refers to any associated types to maybe display a bad conformance diagnostic.

Partially resolves SR-8813. (Arguably, we should be inferring a valid witness for Value here, but since we aren't, we should definitely be diagnosing it.)

Rather than put the ASTWalker here, would it be better to move it into TypeRepr.cpp via adding a member TypeRepr::visitTypeReprs(llvm::function_ref<void(TypeRepr *)>visitor)?

Relatedly, I noticed that TypeRepr::visitTopLevelTypeReprs() currently has no callers. Should I remove it in this or separate PR?

…as, check to see if it refers to associated types to maybe display a bad conformance diagnostic.
@gregomni
Copy link
Collaborator Author

@swift-ci Please smoke test.

@slavapestov
Copy link
Member

Tests?

@gregomni
Copy link
Collaborator Author

@slavapestov Yep, will do! Have any opinion on the TypeRepr members?

@slavapestov
Copy link
Member

@gregomni After looking at the test case, this feels like a workaround for a bug in associated type inference. I'd rather not add complexity to the type resolution code, which is already very complex, to deal with this particular issue.

foundDC->getDeclaredInterfaceType(),
loc, associatedTypeDecl);
}
if (auto aliasDecl = dyn_cast<TypeAliasDecl>(typeDecl)) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can call maybeDiagnoseBadConformanceRef() unconditionally, if its a typealias in a protocol. It's a hack anyway, so limiting it to direct associated type references isn't necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, that sounds reasonable.

@gregomni
Copy link
Collaborator Author

@swift-ci Please smoke test.

@gregomni
Copy link
Collaborator Author

@swift-ci Please smoke test.

2 similar comments
@gregomni
Copy link
Collaborator Author

@swift-ci Please smoke test.

@gregomni
Copy link
Collaborator Author

@swift-ci Please smoke test.

@slavapestov
Copy link
Member

This is much better, thanks! It looks like you fixed compiler_crashers_2/0127-sr5546.swift as well, do you mind moving it to compiler_crashers_2_fixed? I'll dupe the SRs too.

@gregomni
Copy link
Collaborator Author

@swift-ci Please smoke test.

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

2 participants