Ergonomics of outputting NoThrowResult's manually

[ericphanson]

I have a component that is a NoThrowTransform, and I want to output a NoThrowResult myself, so I can add warnings and violations, so I end my implementation with:

    return NoThrowResult(...result...; warnings, violations)

But I get an error whenever violations is nonempty:

NoThrowResult{Missing}: Transform failed
  ❌ Unexpected violation. Details: ArgumentError("Invalid construction: if `violations` are non-empty, `result` must be `missing`.")

So then I update my code to be:

    if isempty(violations)
        output = ...result...
    else
        # We must not output anything if there have been any violations.
        output = missing
    end
    return NoThrowResult(output; warnings, violations)

However, I think that perhaps TS should handle this for me. E.g. if I pass non-empty violations, I would like TS to just clear the result for me, so I don't need to handle it every time. I suppose specifically I would like

TransformSpecifications.jl/src/nothrow.jl

Lines 75 to 78 in 0101d96

	if !ismissing(result) && !isempty(violations)
	throw(ArgumentError("Invalid construction: if `violations` are non-empty, \
	`result` must be `missing`."))
	end

to change so that violations overrules results, and if there are violations present AND results, then discard the results since we can't trust them, and to not throw an error or warning.

Why? Because often the results and violations are constructed somewhat independently. E.g. something like

result = ...
append!(violations, find_issues(result))
...

If find_issues didn't find anything (and thus returned an empty vector), there's no problem, and if it did, then there are violations. But I feel like TS has all the info to make the correct decision.

I do think this is somewhat more "opinionated". The current design says the user must do all the thinking and pass a consistent output and if they don't then we error. This would say, if the user gives us both violations and a result, prefer to trust the violations. To me that seems pretty safe (it is the conservative choice) and improves the ergonomics (don't need that if block at the end of every component). But I'd love to know what you think @hannahilea.

[hannahilea]

Hrm, this is a good question / good point. The pattern I've used there is to return at the point of violation (early exit), rather than running through to the end of the function. Using your above example:

warnings = String[]

# oh no we caught something bad
bad_thing && return NoThrowResult(; warnings, violations="oh no there was a bad thing")

# continue on...    
output = ...
return NoThrowResult(output; warnings)

Is this sufficient, or do we still need the additional opinionated version you propose? Not sure! I've had some cases where your approach would have made code readability/writability slightly cleaner, but for the most part the pattern here has sufficed. (I can point you at private examples of these via slack, if you'd like.)

For some history (that you did not ask for!), in my original prototype I actually let the user pass an optional result, regardless of whether the violations were empty or not---what if there were intermediate outputs a user wanted captured? I then simplified it to the current state after that approach made return types (and assumptions based on returned types) more confusing than was worth it.

I do think that it would be worth adding a documented pattern (or even a new concrete type) to handle chains (i.e., DAGs where the output of each step is the input to the next step) with return type NoThrowResult, for cases (like the one I suspect you're currently working through) where a step is full of lots of intermediate checks for violations.

[ericphanson]

I actually prefer to not early exit within a component unless we need to, because if there's more than one violation, it's nicer for the consumer to see them all. Often we can't (e.g. can't process something that doesn't exist), but when we can I think it's slightly better. Thus IMO early-exiting doesn't quite solve the problem.

I actually let the user pass an optional result, regardless of whether the violations were empty or not---what if there were intermediate outputs a user wanted captured? I then simplified it to the current state after that approach made return types (and assumptions based on returned types) more confusing than was worth it.

This seems like a good choice to me. I think dealing w/ possibly invalid state can be very error-prone and it's nicer to avoid needing to think about invalid state across components. (But within a component, I think it can be up to the component author if they want to bail early or think it's OK to finish out the component).

[hannahilea]

because if there's more than one violation, it's nicer for the consumer to see them all

I think that's true, but also multiple independently-assessable violations is not necessarily the most common case, and when it is the case, I don't know that it is that much extra work to push to a violations vector that can be returned whenever ready...?

But within a component, I think it can be up to the component author if they want to bail early or think it's OK to finish out the component

Yeah, I agree. If this is very common, I guess I don't object to adding the opinionated version that you propose; I also don't know that I think

return isempty(violations) ? NoThrowResult(foo; warnings) : NoThrowResult(; violations, warnings)

is too unreadable. I don't feel strongly about this; if you do, open a PR for it and I'll not oppose it. :)