Skip to content

Correct discrepancies in the package interface file lookup. #75160

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 2 commits into from
Jul 12, 2024

Conversation

elsh
Copy link
Contributor

@elsh elsh commented Jul 11, 2024

Add a check for the client side flag which explcitly opts in to loading the package interface, besides whether package-name is empty or in the same package.

rdar://131393508

Add a check for the client side flag which explcitly opts in to loading the package interface,
besides whether package-name is empty or in the same package.

rdar://131393508
@elsh elsh requested a review from cachemeifyoucan July 11, 2024 01:30
@elsh elsh requested a review from artemcm as a code owner July 11, 2024 01:30
@elsh
Copy link
Contributor Author

elsh commented Jul 11, 2024

@swift-ci smoke test

@elsh
Copy link
Contributor Author

elsh commented Jul 11, 2024

@cachemeifyoucan Could you confirm these lines still belong to the same enclosing if block that's now modified (L672)?

    if (!ctx.LangOpts.AllowNonPackageInterfaceImportFromSamePackage) {
      if (auto packageName = getPackageNameFromInterface(interfacePath, fs)) {
        if (*packageName == ctx.LangOpts.PackageName)
          return std::nullopt;
      }
    }

@cachemeifyoucan
Copy link
Contributor

cachemeifyoucan commented Jul 11, 2024

I feel like the correct behavior for a client in the same package is to load the binary module if package interface should not be loaded, not fallback to public/private interface, isn't it?

@elsh
Copy link
Contributor Author

elsh commented Jul 11, 2024

I feel like the correct behavior for a client in the same package is to load the binary module if package interface should not be loaded, not fallback to public/private interface, isn't it?

The change is part of findInterfacePath so whether a binary module should be loaded is decided elsewhere. Even if the imported module is in the same package, it's not necessary that it needs to be loaded as a binary module. If the module does not contain package decls and package interface path can't be returned due to missing flags, public or private interface should be loaded even if it's in the same package.

@cachemeifyoucan
Copy link
Contributor

I feel like the correct behavior for a client in the same package is to load the binary module if package interface should not be loaded, not fallback to public/private interface, isn't it?

The change is part of findInterfacePath so whether a binary module should be loaded is decided elsewhere.

The difference is that if an interface is found (depending on the priority of the loading order for binary vs. interface), binary module will not be attempted to be loaded. Thus this function actually partially decide if binary module should be used or not in certain situations.

If the module does not contain package decls and package interface path can't be returned due to missing flags, public or private interface should be loaded even if it's in the same package.

As long as you think it is ok to fall back to public/private interface, risking not finding the package specific interfaces, this change is good to me.

Copy link
Contributor

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

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

Can you please add a test?

@elsh elsh requested a review from xymus as a code owner July 11, 2024 20:13
@elsh
Copy link
Contributor Author

elsh commented Jul 11, 2024

@swift-ci smoke test

@elsh
Copy link
Contributor Author

elsh commented Jul 12, 2024

I feel like the correct behavior for a client in the same package is to load the binary module if package interface should not be loaded, not fallback to public/private interface, isn't it?

The change is part of findInterfacePath so whether a binary module should be loaded is decided elsewhere.

The difference is that if an interface is found (depending on the priority of the loading order for binary vs. interface), binary module will not be attempted to be loaded. Thus this function actually partially decide if binary module should be used or not in certain situations.

It can return null in certain situations as you have added per if (!ctx.LangOpts.AllowNonPackageInterfaceImportFromSamePackage) ... so the caller can make a decision on what module to use, but the if block needs to be guarded by a check that the client also opts in to load package interface; otherwise when private or public interface (in the same package) are expected to be loaded, they'll never be loaded.

If the module does not contain package decls and package interface path can't be returned due to missing flags, public or private interface should be loaded even if it's in the same package.

As long as you think it is ok to fall back to public/private interface, risking not finding the package specific interfaces, this change is good to me.

Requiring the client flag (to load package interface) for finding the path for the package interface makes it less error-prone, since otherwise client without the client flag can't even load that package even if the interface found is the package interface. Note that disable-print-package-name-for-non-package-interfacewas introduced recently to migrate package-name off of private or public interfaces.

@elsh elsh merged commit 8bb6b30 into main Jul 12, 2024
3 checks passed
@elsh elsh deleted the elsh/lookup-pkg-interface branch July 12, 2024 18:56
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.

3 participants