Rewrite annotated to apply annotations in certain order

[tchung1118]

Currently all annotations that are passed to fx.Annotate are applied at once when annotated.Build() is called.
This has led to an issue where lifecycle hook annotations (fx.OnStart and fx.OnStop) do not work properly when
used with other parameter/result annotations. (#937)

This PR splits up existing code that applies annotations and makes annotated.Build() apply each annotation
one by one, mostly in the order that they're passed in. In the process of applying individual annotations in order,
it puts lifecycle hook annotations aside and applies them after all the other annotations are applied.

Also, lifecycle hook annotations have been changed so that it can now take zero or more of the following as their arguments:

• parameters of the annotated function
• results of the annotated function
• an fx.Lifecycle object
• a context.Context object

In other words, lifecycle hook annotations can no longer pull in extra dependencies outside of things on which
the annotated constructor is dependent, results that the annotated constructor provides, context.Context
object which is injected by Lifecycle, and the Lifecycle object itself.

[codecov]

Codecov Report

Merging #983 (97ecbb9) into master (b2c7f3c) will decrease coverage by 0.25%.
The diff coverage is 98.83%.

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
- Coverage   98.81%   98.56%   -0.26%     
==========================================
  Files          39       39              
  Lines        1608     1816     +208     
==========================================
+ Hits         1589     1790     +201     
- Misses         14       18       +4     
- Partials        5        8       +3     

Impacted Files	Coverage Δ
annotated.go	98.88% <98.83%> (-0.81%)	⬇️
app.go	96.23% <0.00%> (-1.08%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[tchung1118]

Wrap this error for nicer format

[sywhang]

Took a first pass

[JacobOaks]

This return isn't technically necessary. Also, I don't think this reads as well as how you have it currently, but if you combine the ifs into an if / else if, neither return is necessary, can just rely on the one under these two ifs. Not really sure what the preference for this kind of thing is.

[sywhang]

You can't combine these ifs. While they look like conditionals, they're not conditionals.

These two things are in plain English:

1. If you try providing the constructor to the scope, and it fails, set the result to an error and return
2. Then try Invoking this function, and if it fails, set the result to another error and return.

2 is dependent on 1 succeeding so it's not an if-else clause.

[JacobOaks]

I understand wanting to keep it the way it is. But isn't this technically the same behavior:

if err := scope.Provide(ctor); err != nil {
    results[0] = reflect.ValueOf(fmt.Errorf("Error providing possible parameters for hook installer: %w", err))
} else if err := scope.Invoke(invokeFn.Interface()); err != nil {
    results[0] = reflect.ValueOf(fmt.Errorf("Error invoking hook installer: %w", err))
}
return

Currently:

1. If providing the constructor to the scope fails, we set result to an error and return.
2. Otherwise, we try to invoke the function. If this fails, we set the result to an error and return.
3. Finally, if both of those succeed, we return.

With an if / else if clause:

1. If providing the constructor to the scope fails, we set the result to an error. We do not interact with the else if, and jump straight over it to the return statement and return
2. Otherwise, we try invoking the function. If that fails, we set the result to another error. We exit the else if, and return
3. If both succeed, we return.

Aren't these the same?

[sywhang]

ah, I see what you mean. You're right, it is possible to remove the returns by changing this to else if.

That being said, I don't think this necessarily helps the readability of the code.

Here are some thoughts behind that:

1. if-else statements are usually mutually exclusive statements. i.e.

if A { 
  // something 
} else if B {
  // another thing
} else {
  // something else 
}

In the code above, we take "A/something", "B/another thing", and "C/something else" are all mutually exclusive statements that don't seem correlated. So if we grouped this together, it makes it seem like Provide and Invoke are mutually exclusive.

2. That being said, you could argue that the thing that corresponds to A/B/C above in this code is failure cases , which technically are mutually exclusive - you can't fail in both cases.

3. With these in mind, the code here is doing a sequence of actions, while checking for failures in each action. It's more natural to not group them under an if-else.

You can see examples of code like this in the standard library as well where they check for errors and return early if an error is encountered using series of if statements instead of an if-else statement. Example: https://github.com/golang/go/blob/master/src/internal/poll/fd_unix.go#L369.

[JacobOaks]

Ah okay. Yeah, I agree it's more readable how it is now. Cool, thanks for the standard library example.

[sywhang]

LGTM other than some outlying things re: testing and some updated docstring. We can do some more post cleanups in smaller chunks.

[tchung1118]

Also I just tested, and I think this PR also implemented suggestion mentioned in #936.

[jkanywhere]

Misspelled lifecylce.

Also this diff replaces OnStart with OnStop so OnStop is used twice in the sentence. The words "other types" makes me think it should be OnStop followed by OnStart as before this diff.

"Only one OnStop annotation ... however functions may be annotated with other types of lifecylce Hooks, such as OnStop"