stjude-rust-labs / wdl

Rust crates for working with Workflow Description Language (WDL) documents.
Apache License 2.0
24 stars 6 forks source link

Implement "unused" analysis warnings #200

Open peterhuene opened 2 days ago

peterhuene commented 2 days ago

With static analysis mostly completed with #199, it's time to add additional warnings to analysis:

miniwdl has this warning, which I'm not entirely sure we need to implement or not:

a-frantz commented 2 days ago

Should any/all of these be packaged under wdl-lint? If not, where do they go? Can they be silenced with lint directives? (If yes, we need to rename them 😭)

As for unused_output, I can see some cases where it should be enabled, but I would have it off by default. So I don't consider it a high priority to get implemented. In fact I wouldn't implement this one until we introduce configuration of some sort.

A place I'd enable unused_output is our make_reference workflows. But for our *-standard workflows, we always ignore this warning from miniwdl (and tbh I find it annoying I can't silence it)

peterhuene commented 2 days ago

All good questions.

Of these warnings, only unused declaration would be able to be implemented as a lint we can build an evaluation graph solely from the AST.

The other two require being able to resolve imports and type checking (e.g. if an import defines a struct X and we saw an expression with an X literal, that would count as a "use" of X and thus its import), which only occurs in wdl-analysis.

With a refactoring such that wdl-lint is wdl-analysis-bsaed rather than wdl-ast-based, we could implement all of them as lints, which would solve the "how do we disable them?" question.

I'd have to give that more consideration, though.

a-frantz commented 2 days ago

With a refactoring such that wdl-lint is wdl-analysis-bsaed rather than wdl-ast-based, we could implement all of them as lints, which would solve the "how do we disable them?" question.

Unless there's a rationale I'm missing, this seems like a no-brainer. The way I'm looking at it, the more information we have available to write lints for, the better. Maybe there's an argument that needlessly complicates lints? Plus I think there's some rules in the "baseline lint rules" that require this information. Granted, those could always be "upgraded" out of wdl-lint if we wanted to keep it ast-based.

peterhuene commented 2 days ago

Let me think through what information we could provide to wdl-lint to make this work; with the current PR, the evaluation graphs are thrown away after analysis, so we'd need to keep them around (one for each workflow and task in the document graph) at a bare minimum.

With wdl-lint originally designed before wdl-analysis existed and wdl-analysis designed to answer requests for wdl-lsp, this might take quite a bit of doing.

a-frantz commented 2 days ago

A nagging thought before I put this away for the weekend: are there any warning or note diagnostics currently emitted by wdl-analysis?

A potential compartmentalization is to say "warnings and notes are always considered lints (and are therefore the domain of wdl-lint)". Then analysis "and below" only concern themselves with "serious" errors.

peterhuene commented 1 day ago

Currently wdl-analysis only emits error diagnostics.