-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[cxx-interop] Fix spurious ambiguous member lookup for eagerly imported members #78673
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
Conversation
|
@swift-ci Please smoke test |
Xazax-hun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor questions inline but looks good to me overall.
lib/ClangImporter/ClangImporter.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this dyn_cast could ever fail. Do you know a DeclContext that is not a declaration itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what cases will this check be triggered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great question... and I'm not actually sure. This logic is copied from the original implementation.
I'll try to get to the bottom of this before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I think this check is redundant, because ClangDirectLookupRequest is already checking this: https://github.com/swiftlang/swift/blob/d50faa8886fcf9213adfef23ecaf6714d5bcfa99/lib/ClangImporter/ClangImporter.cpp#L5028-L5043
where isDirectLookupMemberContext() is: https://github.com/swiftlang/swift/blob/d50faa8886fcf9213adfef23ecaf6714d5bcfa99/lib/ClangImporter/ClangImporter.cpp#L4982-L5003
If anything, ClangDirectLookupRequest seems to be somewhat more thorough and accounting for things like canonical decls and namespaces (though they shouldn't be relevant here), so the check here is more restrictive ClangDirectLookupRequest for reasons that aren't obvious to me (and there don't seem to be clues in the commit history either).
I'm going to remove this check and be ready to revert it in case there's some edge case this check was supposed to account for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whelp, that check was doing something after all. It seemingly prevents a crash from happening in this test: https://github.com/swiftlang/swift/blob/439f7d9b672ab2072ad458900752c39c171fd7f7/test/ClangImporter/enum-anon.swift
I'll revert that change.
egorzhdan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked into the implementation bits super closely, but this LGTM in principle, thanks!
|
@swift-ci Please test |
|
@swift-ci please test |
|
Not sure why that failed. Relevant output that I could find: Gonna try this one more time |
|
@swift-ci Please test |
|
@swift-ci please smoke test |
Doing so avoids some annoyingly obscure false-positive errors
439f7d9 to
dd29200
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test linux |
This should have been fixed in #78673 but it was missed because it requires a fairly complex trigger. The fix is to ensure that we never try to clone inherited methods that are themselves inherited.
Follow-up from #78132, which did not fix issues related to eagerly imported members like subscripts.
This patch restructures recursive ClangRecordMemberLookup requests to
importBaseMemberDecl()in the recursive calls, rather than propagating base member decls up to the initial lookup request and doing the import. Doing so seems to fix lingering resolution issues (which I've added to the regression tests).rdar://141069984