Skip to content

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Mar 27, 2020

These commits contain:

  • Make the few places we have requests that just execute side effects painfully clear with the new evaluator::SideEffect return type - an alias to std::tuple<>
  • Strip the evaluation points of llvm::Expected. Request evaluation itself may still fail with an error - that is a valuable behavior to preserve because it means we can still perform rich error reporting and handling during request evaluation. But the evaluation functions themselves have no need to return error values as they should be pure functions of their input state. The evaluator framework itself handles the actual cycle detection and request cancellation portions just fine.

A few requests that were taking advantage of being able to cancel cycles by bubbling errors back up were reverted to their behavior before #18454.

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci test

@swiftlang swiftlang deleted a comment from swift-ci Mar 27, 2020
@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci test

1 similar comment
@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci test

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci test Windows

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 040366d70881d11ac957374de482f72ddf074c3a

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 040366d70881d11ac957374de482f72ddf074c3a

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci please smoke test

1 similar comment
@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci please smoke test

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci please smoke test

1 similar comment
@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci please smoke test

Introduce evaluator::SideEffect, the type of a request that performs
some operation solely to execute its side effects. Thankfully, there are
precious few requests that need to use this type in practice, but it's
good to call them out explicitly so we can get around to making them
behave much more functionally in the future.
Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

Looks good, but I would prefer it if SideEffect was its own type and not a typealias for std::tuple<>.

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

Due to the incredible subtleties of C++, this was actually quite the behavior change. I accidentally list-initialized the Optional part of Optional<std::tuple<>> instead of the tuple part, which forced the evaluator to run back through a raw value request that had already failed which caused the tests to fall over.

A request is intended to be a pure function of its inputs. That function could, in theory, fail. In practice, there were basically no requests taking advantage of this ability - the few that were using it to explicitly detect cycles can just return reasonable defaults instead of forwarding the error on up the stack.

This is because cycles are checked by *the Evaluator*, and are unwound by the Evaluator.

Therefore, restore the idea that the evaluate functions are themselves pure, but keep the idea that *evaluation* of those requests may fail. This model enables the best of both worlds: we not only keep the evaluator flexible enough to handle future use cases like cancellation and diagnostic invalidation, but also request-based dependencies using the values computed at the evaluation points. These aforementioned use cases would use the llvm::Expected interface and the regular evaluation-point interface respectively.
@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci please smoke test

1 similar comment
@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci please smoke test

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 040366d70881d11ac957374de482f72ddf074c3a

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 040366d70881d11ac957374de482f72ddf074c3a

@CodaFi
Copy link
Contributor Author

CodaFi commented Mar 27, 2020

⛵️

@CodaFi CodaFi merged commit 1f904ca into swiftlang:master Mar 27, 2020
@CodaFi CodaFi deleted the consteval branch March 27, 2020 16:43
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.

4 participants