Skip to content

[CXX Interoperability] Stop re-importing FuncDecl when FuncDecl is imported before parent record #59607

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 4 commits into from
Jun 22, 2022

Conversation

Huddie
Copy link
Contributor

@Huddie Huddie commented Jun 21, 2022

We saw a test case failing when 2 records contain the same operator. This occurs because when the first operator is called, we import the record associated with that operator but we also import the function for the 2nd record. So if we have 2 records Foo and Bar and both implement operator-, after calling Foo's operator- we would have imported

  1. Foo
  2. Foo.operator-
  3. Bar.operator-

Then when we call Bar.operator- we try importing Bar record & then import the operator again. So that ends up with

  1. Foo
  2. Foo.operator-
  3. Bar.operator-
  4. Bar
  5. Bar.operator-

which causes there to be 2 imports of the same operator (FuncDecl)

This patch checks to see if the FuncDecl was previously imported and returns early if it has been

Thanks @egorzhdan and @zoecarver for helping me debug this one :p

@Huddie Huddie requested review from egorzhdan and zoecarver June 21, 2022 16:44
@Huddie Huddie added the c++ interop Feature: Interoperability with C++ label Jun 21, 2022
@Huddie
Copy link
Contributor Author

Huddie commented Jun 21, 2022

@swift-ci please smoke test

Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

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

Really glad you figured it out! Thanks, Ehud!

@egorzhdan
Copy link
Contributor

Looks great, thanks!
@Huddie can you please remove the merge commit from this branch? It will mess up git blame if we merge this. You can rebase your branch onto main instead.

@Huddie Huddie force-pushed the address-only-operators branch from 13a9b90 to 9b11bff Compare June 21, 2022 19:11
@Huddie
Copy link
Contributor Author

Huddie commented Jun 21, 2022

@swift-ci please smoke test

@Huddie
Copy link
Contributor Author

Huddie commented Jun 21, 2022

@swift-ci please smoke test

@Huddie
Copy link
Contributor Author

Huddie commented Jun 22, 2022

@swift-ci please test windows

@Huddie
Copy link
Contributor Author

Huddie commented Jun 22, 2022

@swift-ci please smoke test

Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

Thanks!

@egorzhdan egorzhdan merged commit 23f5e01 into swiftlang:main Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants