stjude-rust-labs/wdl

Fix how exceptions work

Closed this issue · 1 comments

In #130, we discovered a bug in how excepting rules work. Here's the description of the problem as posted in Slack and also the suggested fix:

The rule actually works if you remove the except:. As has been hinted, adding the except statement means that the callback for command_text is never called. That, in turn, means that only newlines before and after the command segment are respected, and this behavior of skipping the processing of callbacks for excepted rules is a problem for rules that need to keep track of state—even if they don't report any diagnostics in that area.

One solution is to simply make available the information for every callback whether or not the current callback is excepted. I think this is less optimal for a variety of reasons.

A better approach would probably be locking of the Diagnostics object so that no new diagnostics can be added during a rule's excepted period. In that way, designers of rules wouldn't have to always be thinking about whether things are excepted: they'd just call state.add(_) and that would simply fail to add the diagnostic. Of course, that method may need to be renamed given that it would no longer be guaranteed, and we might have some result indicating back to the rule writer if the add was successful or not.

@a-frantz also suggested tracking whether or not expected exceptions had an effect and flagging exceptions that had no effect.

Once this is finished, the tests introduced in #130 should change, and only the offending line should be flagged as too long (instead of the entire command block).

"double excepting" a rule should be possible, but it currently is not. By "double excepting", I mean having a lint directive comment disabling as well as passing that rule to sprocket --lint except. Currently that leads to this behavior:

note: unknown lint rule `LineWidth`
    ┌─ ./workflows/qc/quality-check-standard.wdl:110:20
    │
110 │         #@ except: LineWidth
    │                    ^^^^^^^^^ cannot make an exception for this rule
    │
    = fix: remove the rule from the exception list

We need to upgrade the unknown lint rule diagnostic to a "full-fledged" rule, (UnknownRule) that can itself be excepted. And that new UnknownRule lint needs to be "smarter" about how it checks for possible rules. Instead of checking against the currently enabled rules, it needs to check against the possible rules including ones which are currently off.