-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[SE-0491] Conditionally emit module selectors into swiftinterfaces #85043
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
lib/AST/ASTPrinter.cpp
Outdated
| if (M->isBuiltinModule()) | ||
| return true; | ||
|
|
||
| // Don't module-qualify a local type. |
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.
Why not?
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.
Module selectors skip directly to module-scope lookup, so a reference qualified by a module selector won't find a local type.
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.
Ah, that makes sense. Worth updating the comment then, since it would have avoided my question? :)
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.
This comment now reads:
// Module selectors skip over local types, so don't add one.
8e2c663 to
a3e74fe
Compare
a3e74fe to
ba3cdd9
Compare
tshortli
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.
🎉
include/swift/Option/Options.td
Outdated
| def module_interface_qualification : | ||
| Separate<["-"], "module-interface-qualification">, | ||
| Flags<[FrontendOption, NoInteractiveOption]>, | ||
| MetaVarName<"<none|alias|selector|conditional>">, |
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.
Minor: I'd expect conditional to be something like selector-conditional
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.
Honestly, all of these names are a little weird; I might workshop them a bit before I commit to them.
| ModuleDecl *M) { | ||
| PrettyStackTraceDecl stackTrace("emitting swiftinterface for", M); | ||
|
|
||
| assert(M); |
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.
Any reason not to use ASSERT here instead?
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.
The original code didn't, but I might as well upgrade it.
This support is currently opt-in and can be disabled by a blocklist.
ba3cdd9 to
9454c0a
Compare
|
@swift-ci please test |
|
@swift-ci please test |
Adds a new frontend and legacy driver flag,
-{enable,disable}-module-selectors-in-module-interface, to control whether decl references in module interfaces are qualified using member syntax (the previous behavior) or module selectors:Currently,
disableis the default; in the future, it will becomeenable. Also includes support for disabling using the blocklist actionDisableModuleSelectorsInModuleInterface.Continuation of #34556.