Skip to content

Use Disjunction Constraint to find main function #58429

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 6 commits into from
Apr 28, 2022

Conversation

etcwilde
Copy link
Member

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.

@main
struct MainType : App {
#if CONFIG1
    typealias Configuration = Config1
#elseif CONFIG2
    typealias Configuration = Config2
#elseif CONFIG3
    typealias Configuration = Config3
#endif
}

protocol AppConfiguration { }

struct Config1: AppConfiguration {}
struct Config2: AppConfiguration {}
struct Config3: AppConfiguration {}

protocol App {
    associatedtype Configuration: AppConfiguration
}

extension App where Configuration == Config1 {
    static func main() { print("Config1") }
}

extension App where Configuration == Config2 {
    static func main() async { print("Config2") }
}

extension App {
    static func main() async { print("Default Async") }

    static func main() { print("Default Sync") }
} 

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 from Config1.

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, called main. 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.

@etcwilde etcwilde requested a review from xedin April 26, 2022 23:13
@etcwilde
Copy link
Member Author

@swift-ci please test

@etcwilde etcwilde requested a review from DougGregor April 26, 2022 23:13
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.
@etcwilde etcwilde force-pushed the ewilde/disjunction-main-resolution branch from 5407c96 to 1ed8556 Compare April 26, 2022 23:30
@etcwilde
Copy link
Member Author

@swift-ci please test

@etcwilde etcwilde requested a review from xedin April 27, 2022 20:20
@etcwilde
Copy link
Member Author

@swift-ci please test

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

Look

// CHECK-CONFIG3: [[SOURCE_FILE]]:[[# @LINE+1 ]]
static func main() { }
extension App {
static func main() async { }
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only expected behavior change?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's more behavior change here than just the ambiguous resolution with both main functions.
The bigger change here is that we always resolve the most specific main function regardless of whether it's async or sync without needing the flag to bias things.

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.
@etcwilde etcwilde force-pushed the ewilde/disjunction-main-resolution branch from 28459a6 to b9405bb Compare April 28, 2022 01:01
@etcwilde
Copy link
Member Author

@swift-ci please test

@etcwilde etcwilde requested a review from xedin April 28, 2022 15:29
@etcwilde etcwilde merged commit 8b2ccd9 into swiftlang:main Apr 28, 2022
@etcwilde etcwilde deleted the ewilde/disjunction-main-resolution branch April 28, 2022 16:31
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