-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[🍒5.7] Use Disjunction Constraint to find main function #58489
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
etcwilde
merged 6 commits into
swiftlang:release/5.7
from
etcwilde:ewilde/disjunction-main-resolution
Apr 28, 2022
Merged
[🍒5.7] Use Disjunction Constraint to find main function #58489
etcwilde
merged 6 commits into
swiftlang:release/5.7
from
etcwilde:ewilde/disjunction-main-resolution
Apr 28, 2022
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit da0a331.
The main function is different from other function resolutions. It isn't being called from a synchronous or asynchronous context, but defines whether the program starts in a synchronous or async context. As a result, we should not prefer one over the other, so the scoring mechanism shouldn't involve the async/sync score when resolving the main function. This patch adds a constraint solver flag to ignore async/sync context mismatches so that we do not favor one over the other, but otherwise use the normal resolution behavior.
Using `resolveValueMember` can result in the wrong, or unrelated main functions being selected. If an error occurs while resolving, such as an ambiguous resolution, the solution will contain everything called 'main'. During the resolution, the `BestOverload` will not be set, so the code skips down to iterating over the member decls and selecting the first declaration that is a `MainTypeMainMethod`. The order of candidates in the failure state is related to the order that the declarations appear in source. The selected and potentially unrelated main function is called from `$main`. The call in the expression will then detect the invalid state later. When only a single main function could exist in a given context, this was not a problem. Any other possible solutions would result in an error, either due to a duplicate declaration, or from calling an unrelated function. When we introduced the asynchronous main function, the resolution can be ambiguous because we allow asynchronous overloads of synchronous functions. This enables us to legally write ``` struct MainType { static func main() { } static func main() async { } } ``` From the perspective of duplicate declarations, this is not an issue. It is, however, ambiguous since we have no calling context with which to break the tie. In the original code, this example would select the synchronous main function because it comes first in the source. If we flip the declarations, the asynchronous main function is selected because it comes first in the source. Instead, using the constraint solver to solve for the correct overload will ensure that we only get back a valid main function. If the constraint solver reports no solutions or many solutions, we can emit an error immediately, as it will not consider unrelated functions.
Changing the overload resolution behavior results in the ordering that `main` and `$main` are typechecked. If there is an error in parsing or sema before we have typechecked these two functions, we get weird errors about `$main` calling the main-actor `main` function being invalid. This is because we have already set the `@main` attribute to invalid, so we don't recognize the `$main` context as being a main entrypoint. The error message compounds to making a confusing situation worse given that `$main` is implicit. So by ignoring the failed `@main` attribute, we still annotate the `$main` and `main` function as being on the main actor, so everyone is happy in that respect. To get the nasty behavior, you can forget to pass `-parse-as-library` with this commit removed.
This patch contains the updates for the tests. The merge removes the `-async-main` flag, so the tests using that flag fail on that. Additionally, the tests reflect the newer behavior of the main resolution. `async_main_resolution` verifies that we're not preferring an async function over a synchronous function, and vice versa. This is also verified in `where_clause_main_resolution`, where we select a main function using various configuration typealiases. Finally, we only select a valid, usable main function. The old machinery could select one sort of at random (technically it selected the first overload declared in the source file) if an error occurred. Errors that resulted in this behavior included a missing valid function, or an ambiguous overload. In the case of the missing valid function, the function returned would be from an invalid location if one existed, which was called from `$main`. `$main` had sufficient context to realize that the main function being called was not valid, and would emit an error saying why. It would be better to realize that we're not getting a valid main function earlier, and later we can emit notes saying why each function called main could not be selected.
This sets up the type constraints as equalities with a type variable. This is apparently more performant in the solver, so that should be good. It doesn't have additional effect on the resulting behavior.
@swift-ci please test |
hborla
approved these changes
Apr 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cherry-picking #58429.
Third time trying to get the main function resolution behaving correctly.
The challenge here is that, unlike other functions, we don't have a way to determine whether the "calling context" of the main function is synchronous or asynchronous because there isn't a calling context. It is up to the main function to determine whether the rest of the program is run from a sync or async context.
The last two designs didn't work correctly for various reasons.
Simply making the constraint solver ignore async/sync mismatches results in incorrect behavior with conditional conformances. The following snippet exposed the issue. The original resolver would fall back on grabbing the first candidate from the list of candidates if no best solution was found. A best solution wouldn't be set if there were ambiguities or missing functions. The candidates were in the order of declaration in the source file. So in this case, if you selected configuration 3, it would see the ambiguity between the synchronous and asynchronous main function, and select the main function in config 1. (This is bad!) It should instead report the ambiguity.
To mitigate this, I tried using an
async-main
flag, which would bias the constraint solver in favor of one over the other.This was not ideal for a few reasons. While it resolved the above case with the explicit flag, it first, required an explicit flag that would completely change the behavior of the program, but second, would prefer either the sync or async function of the specific type. e.g, in the above case, if you specified
Config1
, with the async flag selected, it would choose the default async main function instead of the function fromConfig1
.In order to actually fix this to be properly sane, I've done a bit of digging with the constraint solver. Rather than dragging out everything called
main
, then trawling the list of everything we found, I've set up a disjunction constraint of only the types and names of valid main functions. This way we only get a list of valid function declarations, with a valid type, calledmain
. The valid types are() -> Void
,() async -> Void
,() throws -> Void
,() async throws -> Void
,@MainActor () -> Void
,@MainActor () async -> Void
,@MainActor () throws -> Void
, and@MainActor () async throws -> Void
.rdar://89500797