Skip to content

Conversation

@elsh
Copy link
Contributor

@elsh elsh commented Sep 27, 2023

Package decls are only printed in interface files if they are inlinable
(@usableFromInline, @inlinable, @_alwaysEmitIntoClient). They could be
referenced by a module outside of its defining module that belong to the same
package determined by the package-name flag. However, the flag is only in
.swiftmodule and .private.swiftinterface, thus type checking references of
inlinable package symbols in public interfaces fails due to the missing flag.
Instead of adding the package-name flag to the public interfaces, which
could raise a security concern, this PR grants access to such cases.

Resolves rdar://116142791

@elsh
Copy link
Contributor Author

elsh commented Sep 27, 2023

@swift-ci smoke test

@tshortli
Copy link
Contributor

I think we should strive to hide package names from public .swiftinterfaces. What if we just carved out a typechecking exception for @usableFromInline package decls in .swiftinterfaces instead, allowing them to be present without knowing the package name?

@elsh
Copy link
Contributor Author

elsh commented Sep 28, 2023

I think we should strive to hide package names from public .swiftinterfaces. What if we just carved out a typechecking exception for @usableFromInline package decls in .swiftinterfaces instead, allowing them to be present without knowing the package name?

That was my initial thought but after having discussed more with @xymus this way seemed simpler (so users don't have to figure out why they sometimes see package-name and sometimes don't). I'm open to adding more restrictions though.

@elsh
Copy link
Contributor Author

elsh commented Sep 28, 2023

@swift-ci smoke test

@tshortli
Copy link
Contributor

I don't think we should be too concerned about users being confused about why -package-name is absent from a public .swiftinterface; most developers have no reason to inspect a .swiftinterface and ultimately they are a compiler implementation detail. The typechecking rule that requires -package-name be specified when using package decls is meant to help developers avoid mistakenly forgetting to configure their package/project properly for package use. That's useful when typechecking source but I don't think the diagnostic makes as much sense in the context of typechecking a swiftinterface. I'd rather we complicate typechecking rules to treat swiftinterfaces differently (we do this a lot already) so that we can avoid leaking unnecessary details in the SDK.

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.

Yay! Thank you Ellie!

@tshortli
Copy link
Contributor

If what I'm suggesting is more complicated than I'm imagining to implement, then a version of this approach that would satisfy me more would be to print a placeholder package name instead of the real package name (ideally the placeholder value is known to the compiler and only accepted when parsing interfaces) .

@xymus
Copy link
Contributor

xymus commented Sep 28, 2023

The way is see the alternative suggested by @tshortli would mean to lift the package restriction when building swiftinterfaces, basically treating them as public at that time. I agree that it should work fine, and the real package boundary is enforced during compilation from source and from private swiftinterface. I expect it to be easy enough to implement at this time where only @usableFromInline and @inlinable package decl are in swiftinterfaces but might require more care when we add a package swiftinterface.

I am not too worried about having the package name in the public swiftinterface at this time either. Only swiftpm automatically sets a package name and we can expect this name to already be public. Adopters in Xcode projects have been setting this value manually. Choosing a package name in Xcode is a similar problem to choosing the module name, leaks can be reported by the same tooling.

My main worry about having the package name in the public swiftinterface is for external clients to accidentally use the same package name. The compiler could then error on the import of a module from the same package rebuilt from the swiftinterface. We have a similar problem with module names again.

@elsh
Copy link
Contributor Author

elsh commented Oct 3, 2023

Modified to print the flag only if the interface contains inlinable (@usableFromInline, @inlinalbe, @_alwaysEmitIntoClient) package decls.

@elsh
Copy link
Contributor Author

elsh commented Oct 3, 2023

@swift-ci smoke test

@elsh elsh changed the title Add package-name flag to public .swiftinterface Print package-name flag in swiftinterface if containing inlinable package decls Oct 3, 2023
@tshortli
Copy link
Contributor

tshortli commented Oct 3, 2023

Modified to print the flag only if the interface contains inlinable (@usableFromInline, @inlinalbe, @_alwaysEmitIntoClient) package decls.

That's an improvement over always printing the package name, but I'd still like to know if just lifting the restriction entirely is a reasonable option so that we never have to print the package name .swiftinterfaces which is what I'd prefer.

@elsh
Copy link
Contributor Author

elsh commented Oct 3, 2023

@swift-ci smoke test

Package decls are only printed in interface files if they are inlinable
(@usableFromInline, @inlinable, @_alwaysEmitIntoClient). They could be
referenced by a module outside of its defining module that belong to the same
package determined by the `package-name` flag. However, the flag is only in
.swiftmodule and .private.swiftinterface, thus type checking references of
inlinable package symbols in public interfaces fails due to the missing flag.
Instead of adding the package-name flag to the public interfaces, which
could raise a security concern, this PR grants access to such cases.

Resolves rdar://116142791
@elsh elsh changed the title Print package-name flag in swiftinterface if containing inlinable package decls Grant access to inlinalbe package symbols referenced in interfaces. Oct 3, 2023
@elsh
Copy link
Contributor Author

elsh commented Oct 3, 2023

Modified to print the flag only if the interface contains inlinable (@usableFromInline, @inlinalbe, @_alwaysEmitIntoClient) package decls.

That's an improvement over always printing the package name, but I'd still like to know if just lifting the restriction entirely is a reasonable option so that we never have to print the package name .swiftinterfaces which is what I'd prefer.

Modified to lift restrictions instead.

@elsh
Copy link
Contributor Author

elsh commented Oct 3, 2023

@swift-ci smoke test

@elsh elsh changed the title Grant access to inlinalbe package symbols referenced in interfaces. Lift restrictions of access check for inlinalbe package symbols referenced in interfaces. Oct 3, 2023
Copy link
Contributor

@tshortli tshortli left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the approach I suggested! Looks like it turned out reasonably straightforward.

@elsh
Copy link
Contributor Author

elsh commented Oct 3, 2023

@swift-ci smoke test

@elsh
Copy link
Contributor Author

elsh commented Oct 4, 2023

Thanks for implementing the approach I suggested! Looks like it turned out reasonably straightforward.

Np! Thanks for suggesting and sticking with the approach so we can keep internal details hidden from users.

@elsh
Copy link
Contributor Author

elsh commented Oct 4, 2023

@swift-ci smoke test windows

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.

5 participants