Skip to content

Make isVisibleFromModule accept TagDecl definitions that were demoted to declarations. #41144

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 16, 2022
Merged

Make isVisibleFromModule accept TagDecl definitions that were demoted to declarations. #41144

merged 2 commits into from
Feb 16, 2022

Conversation

vsapsai
Copy link
Contributor

@vsapsai vsapsai commented Feb 2, 2022

Clang AST requires to have only a single definition in a redeclaration
chain, that's why it demotes duplicate definitions from other modules to
declarations. But on the Swift side it is important to know a module
contains a full definition and not just a forward declaration, even if
it was demoted to a declaration.

@vsapsai
Make isVisibleFromModule accept TagDecl definitions that were demoted…
91a4869 
… to declarations.

Clang AST requires to have only a single definition in a redeclaration
chain, that's why it demotes duplicate definitions from other modules to
declarations. But on the Swift side it is important to know a module
contains a full definition and not just a forward declaration, even if
it was demoted to a declaration.
@vsapsai
Copy link
Contributor Author

vsapsai commented Feb 2, 2022

Please test with following PR:
swiftlang/llvm-project#3872

@swift-ci Please test

@vsapsai vsapsai mentioned this pull request Feb 2, 2022
Merged
@vsapsai
Copy link
Contributor Author

vsapsai commented Feb 2, 2022

I'm not sure what kind of extra testing is required as this change is supposed to preserve the existing behavior. Any feedback about that is welcome.

For now I'm testing with clang change against stable/20211026 The plan is to check that everything works as intended and then to review at https://reviews.llvm.org, land upstream, let it merge to "next", and then cherry-pick to stable/20211026. Also I plan to test this change together with the proposed flag in clang and with "Reland "[modules] Fix miscompilation when using two RecordDecl definitions with the same name."" to make sure this change is sufficient for safe relanding.

@vsapsai vsapsai mentioned this pull request Feb 2, 2022
Closed
xymus
xymus approved these changes Feb 2, 2022
View reviewed changes
Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

That fix brings me hope, thank you!

@xymus
Copy link
Contributor

xymus commented Feb 2, 2022

Could the test modified in #40091 be updated to test this?

etcwilde
etcwilde approved these changes Feb 2, 2022
View reviewed changes
Copy link
Member

@etcwilde etcwilde left a comment

Choose a reason for hiding this comment

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

Yeah, I think this looks good. I agree with @xymus on fixing the test.

@vsapsai
Copy link
Contributor Author

vsapsai commented Feb 2, 2022

Yep, I can totally revert the change in #40091. Just to clarify my understanding, in what cases the test should have failed? Because all PR tests succeed but I'm not sure which ones run "ClangImporter/overlay.swift".

@etcwilde
Copy link
Member

etcwilde commented Feb 3, 2022

Yep, I can totally revert the change in #40091. Just to clarify my understanding, in what cases the test should have failed? Because all PR tests succeed but I'm not sure which ones run "ClangImporter/overlay.swift".

The test uses // expected-error *, so it will accept zero or more of the specified error message. So it kind of just eats the failure. If we get rid of that line, does the test still pass?

@vsapsai
Revert "[ClangImporter] Don't expect unmodularized structs to appear …
0114b4b 
…in 2 modules"

This reverts commit c8c3f9b.

With `isThisDeclarationADemotedDefinition` flag we expect the same
struct to be available in multiple modules.
@vsapsai
Copy link
Contributor Author

vsapsai commented Feb 3, 2022

The test uses // expected-error *, so it will accept zero or more of the specified error message. So it kind of just eats the failure. If we get rid of that line, does the test still pass?

Thanks for the explanation! Totally missed that *. Pushed the revert to PR branch, will rerun the tests.

@vsapsai
Copy link
Contributor Author

vsapsai commented Feb 3, 2022

Please test with following PR:
swiftlang/llvm-project#3872

@swift-ci Please test

@vsapsai
Copy link
Contributor Author

vsapsai commented Feb 16, 2022

isThisDeclarationADemotedDefinition has been added in clang, let me rerun the tests just with commits in this PR, no cross-repo testing.

@vsapsai
Copy link
Contributor Author

vsapsai commented Feb 16, 2022

@swift-ci Please test

@vsapsai vsapsai merged commit 6f9bebe into swiftlang:main Feb 16, 2022
@vsapsai vsapsai deleted the accept-demoted-tagdecl branch February 16, 2022 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xymus xymus xymus approved these changes

@etcwilde etcwilde etcwilde approved these changes

@beccadax beccadax Awaiting requested review from beccadax beccadax is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vsapsai @xymus @etcwilde