Skip to content

Commit

Permalink
feat: implement the ImportPlacement rule. (#89)
Browse files Browse the repository at this point in the history
  • Loading branch information
peterhuene authored Jun 18, 2024
1 parent b33cceb commit 066d6bd
Show file tree
Hide file tree
Showing 16 changed files with 304 additions and 33 deletions.
12 changes: 12 additions & 0 deletions Arena.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ message = "ww-fastq-to-cram.wdl:205:1: warning[EndingNewline]: missing newline a
document = "getwilds/ww-fastq-to-cram:/ww-fastq-to-cram.wdl"
message = "ww-fastq-to-cram.wdl:23:10: warning[SnakeCase]: workflow name `PairedFastqsToUnmappedCram` is not snake_case"

[[diagnostics]]
document = "getwilds/ww-fastq-to-cram:/ww-fastq-to-cram.wdl"
message = "ww-fastq-to-cram.wdl:2:1: note[PreambleWhitespace]: expected a blank line after the version statement"

[[diagnostics]]
document = "getwilds/ww-fastq-to-cram:/ww-fastq-to-cram.wdl"
message = "ww-fastq-to-cram.wdl:57:13: warning[Whitespace]: line contains trailing whitespace"
Expand Down Expand Up @@ -151,6 +155,10 @@ message = "ww-star-deseq2.wdl:255:5: note[MatchingParameterMeta]: task `RNASeQC`
document = "getwilds/ww-star-deseq2:/ww-star-deseq2.wdl"
message = "ww-star-deseq2.wdl:25:36: warning[Whitespace]: line contains trailing whitespace"

[[diagnostics]]
document = "getwilds/ww-star-deseq2:/ww-star-deseq2.wdl"
message = "ww-star-deseq2.wdl:2:1: note[PreambleWhitespace]: expected a blank line after the version statement"

[[diagnostics]]
document = "getwilds/ww-star-deseq2:/ww-star-deseq2.wdl"
message = "ww-star-deseq2.wdl:30:13: warning[Whitespace]: line contains trailing whitespace"
Expand Down Expand Up @@ -355,6 +363,10 @@ message = "ww-vc-trio.wdl:29:10: warning[MatchingParameterMeta]: workflow `ww_vc
document = "getwilds/ww-vc-trio:/ww-vc-trio.wdl"
message = "ww-vc-trio.wdl:2:1: note[PreambleComments]: preamble comments cannot come after the version statement"

[[diagnostics]]
document = "getwilds/ww-vc-trio:/ww-vc-trio.wdl"
message = "ww-vc-trio.wdl:2:1: note[PreambleWhitespace]: expected a blank line after the version statement"

[[diagnostics]]
document = "getwilds/ww-vc-trio:/ww-vc-trio.wdl"
message = "ww-vc-trio.wdl:300:10: warning[MatchingParameterMeta]: task `ApplyBaseRecalibrator` is missing a parameter metadata key for input `input_bam`"
Expand Down
1 change: 1 addition & 0 deletions wdl-ast/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ use std::sync::Arc;
pub use rowan::ast::support;
pub use rowan::ast::AstChildren;
pub use rowan::ast::AstNode;
pub use rowan::Direction;
pub use wdl_grammar::Diagnostic;
pub use wdl_grammar::Label;
pub use wdl_grammar::Severity;
Expand Down
13 changes: 9 additions & 4 deletions wdl-grammar/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

* Fixed the CST for an unparsable file (i.e. one without a supported version)
to contain trivia before the version statement and correct spans for the
unparsed token ([#89](https://github.com/stjude-rust-labs/wdl/pull/89)).
* Fixed last trivia in the file attaching as a child to the last node in the
file instead of as a child of the root ([#89](https://github.com/stjude-rust-labs/wdl/pull/89)).
* Fixed diagnostics around encountering string member names in struct literals
([#87](https://github.com/stjude-rust-labs/wdl/pull/87)).
* Fixed diagnostic label spans that point at strings to include the entire span
of the string ([#86](https://github.com/stjude-rust-labs/wdl/pull/86)).
* Fixed trivia in the CST so that it appears at consistent locations; also
fixed the parser diagnostics to be ordered by the start of the primary label
([#85](https://github.com/stjude-rust-labs/wdl/pull/85)).
* Fixed a missing delimiter diagnostic to include a label for where the parser
* Fixed a missing delimiter diagnostic to include a label for where the parser
thinks the missing delimiter might go ([#84](https://github.com/stjude-rust-labs/wdl/pull/84)).

## 0.4.0 - 6-13-2024

### Changed

* Removed the old parser implementation in favor of the new parser
* Removed the old parser implementation in favor of the new parser
implementation; this also removes the `experimental` feature from the crate ([#79](https://github.com/stjude-rust-labs/wdl/pull/79)).
* Removed dependency on `miette` and `thiserror` in the experimental parser,
introduced the `Diagnostic` type as a replacement, and switched the existing
Expand All @@ -47,7 +52,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
in the experimental parser ([#52](https://github.com/stjude-rust-labs/wdl/pull/52)).
* Fixed parsing of empty inputs to a task call statement in the
experimental parser ([#54](https://github.com/stjude-rust-labs/wdl/pull/54)).
* Fixed parsing of postfix `+` qualifier on array types in the experimental
* Fixed parsing of postfix `+` qualifier on array types in the experimental
parser ([#53](https://github.com/stjude-rust-labs/wdl/pull/53)).
* Fixed parsing of placeholder options in the experimental parser such
that it can disambiguate between the `sep` option and a `sep` function
Expand All @@ -70,7 +75,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
and workflows in the experimental parser ([#39](https://github.com/stjude-rust-labs/wdl/pull/39)).
* Adds support for parsing struct definitions to the experimental parser;
requires the `experimental` feature to be activated ([#38](https://github.com/stjude-rust-labs/wdl/pull/38)).
* Adds a new experimental `SyntaxTree` representation; requires the
* Adds a new experimental `SyntaxTree` representation; requires the
`experimental` feature to be activated ([#36](https://github.com/stjude-rust-labs/wdl/pull/36)).
* Adds an `experimental` module containing the start of a new
infallible WDL parser implementation based on `logos` and `rowan` ([#30](https://github.com/stjude-rust-labs/wdl/pull/30)).
Expand Down
3 changes: 3 additions & 0 deletions wdl-grammar/src/grammar/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,9 @@ pub fn items(parser: &mut Parser<'_>) {
marker.abandon(parser);
}
}

// This call to `next` is important as `next` adds any remaining buffered events
assert!(parser.next().is_none(), "parser is not finished");
}

/// Parses a single top-level item in a WDL document.
Expand Down
28 changes: 24 additions & 4 deletions wdl-grammar/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -447,17 +447,21 @@ where
/// consuming it.
///
/// The token is not added to the event list.
///
/// # Note
///
/// Note that peeking may cause parser events to be buffered.
///
/// If `peek` returns `None`, ensure all buffered events are added to the
/// event list by calling `next` on the parser; otherwise, calling `finish`
/// may panic.
pub fn peek(&mut self) -> Option<(T, Span)> {
while let Some((res, span)) = self.lexer.as_mut()?.peek() {
if let Some(t) = self.consume_trivia(res, span, true) {
return Some(t);
}
}

if !self.buffered.is_empty() {
self.events.append(&mut self.buffered);
}

None
}

Expand Down Expand Up @@ -847,7 +851,19 @@ where
}

/// Consumes the parser and returns the output.
///
/// # Panics
///
/// This method panics if buffered events remain in the parser.
///
/// To ensure that no buffered events remain, call `next()` on the parser
/// and verify it returns `None` before calling this method.
pub fn finish(self) -> Output<'a, T> {
assert!(
self.buffered.is_empty(),
"buffered events remain; ensure `next` was called after an unsuccessful peek"
);

Output {
lexer: self.lexer.expect("lexer should be present"),
events: self.events,
Expand All @@ -874,6 +890,10 @@ where
/// This occurs when a source file is missing a version statement or
/// if the version specified is unsupported.
pub fn consume_remainder(&mut self) {
if !self.buffered.is_empty() {
self.events.append(&mut self.buffered);
}

if let Some(span) = self
.lexer
.as_mut()
Expand Down
4 changes: 2 additions & 2 deletions wdl-grammar/tests/parsing/imports/source.tree
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ RootNode@0..245
Whitespace@165..166 " "
Ident@166..169 "Qux"
Whitespace@169..171 " \n"
ImportStatementNode@171..245
ImportStatementNode@171..244
ImportKeyword@171..177 "import"
LiteralStringNode@177..185
Whitespace@177..178 " "
Expand Down Expand Up @@ -90,4 +90,4 @@ RootNode@0..245
AsKeyword@236..238 "as"
Whitespace@238..239 " "
Ident@239..244 "Corge"
Whitespace@244..245 "\n"
Whitespace@244..245 "\n"
6 changes: 4 additions & 2 deletions wdl-grammar/tests/parsing/missing-version/source.tree
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
RootNode@0..14
Unparsed@0..14 "task foo {\n\n}\n"
RootNode@0..64
Comment@0..48 "# This is a test of a ..."
Whitespace@48..50 "\n\n"
Unparsed@50..64 "task foo {\n\n}\n"
6 changes: 6 additions & 0 deletions wdl-lint/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

### Added

* Added the `ImportPlacement` lint rule ([#89](https://github.com/stjude-rust-labs/wdl/pull/89)).

### Fixed

* Fixed the preamble whitespace rule to check for a blank line following the
version statement ([#89](https://github.com/stjude-rust-labs/wdl/pull/89)).
* Fixed the preamble whitespace and preamble comment rules to look for the
version statement trivia based on it now being children of the version
statement ([#85](https://github.com/stjude-rust-labs/wdl/pull/85)).
Expand Down
25 changes: 13 additions & 12 deletions wdl-lint/RULES.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@ be out of sync with released packages.

## Lint Rules

| Name | Tags | Description |
|:---------------------------------|:------------------------------|:-------------------------------------------------------------------------|
| `CommandSectionMixedIndentation` | Clarity, Correctness, Spacing | Ensures that lines within a command do not mix spaces and tabs. |
| `DoubleQuotes` | Clarity, Style | Ensures that strings are defined using double quotes. |
| `EndingNewline` | Spacing, Style | Ensures that documents end with a single newline character. |
| `MatchingParameterMeta` | Completeness | Ensures that inputs have a matching entry in a `parameter_meta` section. |
| `MissingRuntime` | Completeness, Portability | Ensures that tasks have a runtime section. |
| `NoCurlyCommands` | Clarity | Ensures that tasks use heredoc syntax in command sections. |
| `PreambleComments` | Clarity, Spacing, Style | Ensures that documents have correct comments in the preamble. |
| `PreambleWhitespace` | Spacing, Style | Ensures that documents have correct whitespace in the preamble. |
| `SnakeCase` | Clarity, Naming, Style | Ensures that tasks, workflows, and variables use snake_case. |
| `Whitespace` | Spacing, Style | Ensures that a document does not contain undesired whitespace. |
| Name | Tags | Description |
|:---------------------------------|:------------------------------|:--------------------------------------------------------------------------------------|
| `CommandSectionMixedIndentation` | Clarity, Correctness, Spacing | Ensures that lines within a command do not mix spaces and tabs. |
| `DoubleQuotes` | Clarity, Style | Ensures that strings are defined using double quotes. |
| `EndingNewline` | Spacing, Style | Ensures that documents end with a single newline character. |
| `ImportPlacement` | Clarity | Ensures that imports are placed between the version statement and any document items. |
| `MatchingParameterMeta` | Completeness | Ensures that inputs have a matching entry in a `parameter_meta` section. |
| `MissingRuntime` | Completeness, Portability | Ensures that tasks have a runtime section. |
| `NoCurlyCommands` | Clarity | Ensures that tasks use heredoc syntax in command sections. |
| `PreambleComments` | Clarity, Spacing, Style | Ensures that documents have correct comments in the preamble. |
| `PreambleWhitespace` | Spacing, Style | Ensures that documents have correct whitespace in the preamble. |
| `SnakeCase` | Clarity, Naming, Style | Ensures that tasks, workflows, and variables use snake_case. |
| `Whitespace` | Spacing, Style | Ensures that a document does not contain undesired whitespace. |
3 changes: 3 additions & 0 deletions wdl-lint/src/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::TagSet;
mod command_mixed_indentation;
mod double_quotes;
mod ending_newline;
mod import_placement;
mod matching_parameter_meta;
mod missing_runtime;
mod no_curly_commands;
Expand All @@ -19,6 +20,7 @@ mod whitespace;
pub use command_mixed_indentation::*;
pub use double_quotes::*;
pub use ending_newline::*;
pub use import_placement::*;
pub use matching_parameter_meta::*;
pub use missing_runtime::*;
pub use no_curly_commands::*;
Expand Down Expand Up @@ -68,6 +70,7 @@ pub fn rules() -> Vec<Box<dyn Rule>> {
Box::new(MatchingParameterMetaRule),
Box::new(WhitespaceRule),
Box::new(CommandSectionMixedIndentationRule),
Box::new(ImportPlacementRule),
];

// Ensure all the rule ids are unique and pascal case
Expand Down
121 changes: 121 additions & 0 deletions wdl-lint/src/v1/import_placement.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
//! A lint rule for import placements.
use wdl_ast::v1::ImportStatement;
use wdl_ast::v1::StructDefinition;
use wdl_ast::v1::TaskDefinition;
use wdl_ast::v1::Visitor;
use wdl_ast::v1::WorkflowDefinition;
use wdl_ast::AstNode;
use wdl_ast::Diagnostic;
use wdl_ast::Diagnostics;
use wdl_ast::Span;
use wdl_ast::ToSpan;
use wdl_ast::VisitReason;

use super::Rule;
use crate::Tag;
use crate::TagSet;

/// The identifier for the import placement rule.
const ID: &str = "ImportPlacement";

/// Creates a "misplaced import" diagnostic.
fn misplaced_import(span: Span) -> Diagnostic {
Diagnostic::warning("misplaced import")
.with_rule(ID)
.with_highlight(span)
.with_fix(
"move this import so that it comes after the version statement but before any \
document items",
)
}

/// Detects incorrect import placements.
#[derive(Debug, Clone, Copy)]
pub struct ImportPlacementRule;

impl Rule for ImportPlacementRule {
fn id(&self) -> &'static str {
ID
}

fn description(&self) -> &'static str {
"Ensures that imports are placed between the version statement and any document items."
}

fn explanation(&self) -> &'static str {
"All import statements should follow the WDL version declaration with one empty line \
between the version and the first import statement."
}

fn tags(&self) -> TagSet {
TagSet::new(&[Tag::Clarity])
}

fn visitor(&self) -> Box<dyn Visitor<State = Diagnostics>> {
Box::new(ImportPlacementVisitor::default())
}
}

/// Implements the visitor for the import placement rule.
#[derive(Default)]
struct ImportPlacementVisitor {
/// Whether or not an import statement is considered invalid.
invalid: bool,
}

impl Visitor for ImportPlacementVisitor {
type State = Diagnostics;

fn import_statement(
&mut self,
state: &mut Self::State,
reason: VisitReason,
stmt: &ImportStatement,
) {
if reason == VisitReason::Exit {
return;
}

if self.invalid {
state.add(misplaced_import(stmt.syntax().text_range().to_span()));
}
}

fn struct_definition(
&mut self,
_: &mut Self::State,
reason: VisitReason,
_: &StructDefinition,
) {
if reason == VisitReason::Exit {
return;
}

// Saw an item other than an import, imports are no longer valid
self.invalid = true;
}

fn task_definition(&mut self, _: &mut Self::State, reason: VisitReason, _: &TaskDefinition) {
if reason == VisitReason::Exit {
return;
}

// Saw an item other than an import, imports are no longer valid
self.invalid = true;
}

fn workflow_definition(
&mut self,
_: &mut Self::State,
reason: VisitReason,
_: &WorkflowDefinition,
) {
if reason == VisitReason::Exit {
return;
}

// Saw an item other than an import, imports are no longer valid
self.invalid = true;
}
}
Loading

0 comments on commit 066d6bd

Please sign in to comment.