Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add inconsistent_newlines rule #104

Merged
merged 4 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions Gauntlet.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ commit_hash = "1e32078d3b57dcb2291534d0caa30f600d40967e"

[repositories."broadinstitute/warp"]
identifier = "broadinstitute/warp"
commit_hash = "f094475689311ccf227b39b5f40b70762889566c"
commit_hash = "77a5d123b4aaf44674d9026149442762826b3687"

[repositories."chanzuckerberg/czid-workflows"]
identifier = "chanzuckerberg/czid-workflows"
Expand Down Expand Up @@ -569,37 +569,37 @@ permalink = "https://github.com/broadinstitute/palantir-workflows/blob/1e32078d3
[[diagnostics]]
document = "broadinstitute/warp:/pipelines/broad/dna_seq/somatic/single_sample/wgs/gdc_genome/GDCWholeGenomeSomaticSingleSample.wdl"
message = "GDCWholeGenomeSomaticSingleSample.wdl:672:14: error: conflicting scatter variable name `ubam`"
permalink = "https://github.com/broadinstitute/warp/blob/f094475689311ccf227b39b5f40b70762889566c/pipelines/broad/dna_seq/somatic/single_sample/wgs/gdc_genome/GDCWholeGenomeSomaticSingleSample.wdl/#L672"
permalink = "https://github.com/broadinstitute/warp/blob/77a5d123b4aaf44674d9026149442762826b3687/pipelines/broad/dna_seq/somatic/single_sample/wgs/gdc_genome/GDCWholeGenomeSomaticSingleSample.wdl/#L672"

[[diagnostics]]
document = "broadinstitute/warp:/pipelines/skylab/scATAC/scATAC.wdl"
message = "scATAC.wdl:203:9: error: duplicate key `cpu` in runtime section"
permalink = "https://github.com/broadinstitute/warp/blob/f094475689311ccf227b39b5f40b70762889566c/pipelines/skylab/scATAC/scATAC.wdl/#L203"
permalink = "https://github.com/broadinstitute/warp/blob/77a5d123b4aaf44674d9026149442762826b3687/pipelines/skylab/scATAC/scATAC.wdl/#L203"

[[diagnostics]]
document = "broadinstitute/warp:/tasks/broad/GermlineVariantDiscovery.wdl"
message = "GermlineVariantDiscovery.wdl:137:32: error: expected string, but found integer"
permalink = "https://github.com/broadinstitute/warp/blob/f094475689311ccf227b39b5f40b70762889566c/tasks/broad/GermlineVariantDiscovery.wdl/#L137"
permalink = "https://github.com/broadinstitute/warp/blob/77a5d123b4aaf44674d9026149442762826b3687/tasks/broad/GermlineVariantDiscovery.wdl/#L137"

[[diagnostics]]
document = "broadinstitute/warp:/tasks/broad/GermlineVariantDiscovery.wdl"
message = "GermlineVariantDiscovery.wdl:65:32: error: expected string, but found integer"
permalink = "https://github.com/broadinstitute/warp/blob/f094475689311ccf227b39b5f40b70762889566c/tasks/broad/GermlineVariantDiscovery.wdl/#L65"
permalink = "https://github.com/broadinstitute/warp/blob/77a5d123b4aaf44674d9026149442762826b3687/tasks/broad/GermlineVariantDiscovery.wdl/#L65"

[[diagnostics]]
document = "broadinstitute/warp:/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl"
message = "UltimaGenomicsWholeGenomeGermlineTasks.wdl:814:27: error: expected string, but found integer"
permalink = "https://github.com/broadinstitute/warp/blob/f094475689311ccf227b39b5f40b70762889566c/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl/#L814"
permalink = "https://github.com/broadinstitute/warp/blob/77a5d123b4aaf44674d9026149442762826b3687/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl/#L814"

[[diagnostics]]
document = "broadinstitute/warp:/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl"
message = "UltimaGenomicsWholeGenomeGermlineTasks.wdl:866:27: error: expected string, but found integer"
permalink = "https://github.com/broadinstitute/warp/blob/f094475689311ccf227b39b5f40b70762889566c/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl/#L866"
permalink = "https://github.com/broadinstitute/warp/blob/77a5d123b4aaf44674d9026149442762826b3687/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl/#L866"

[[diagnostics]]
document = "broadinstitute/warp:/tests/cemba/pr/CheckCembaOutputs.wdl"
message = "CheckCembaOutputs.wdl:1:1: error: a WDL document must start with a version statement"
permalink = "https://github.com/broadinstitute/warp/blob/f094475689311ccf227b39b5f40b70762889566c/tests/cemba/pr/CheckCembaOutputs.wdl/#L1"
permalink = "https://github.com/broadinstitute/warp/blob/77a5d123b4aaf44674d9026149442762826b3687/tests/cemba/pr/CheckCembaOutputs.wdl/#L1"

[[diagnostics]]
document = "chanzuckerberg/czid-workflows:/workflows/index-generation/index-generation.wdl"
Expand Down
1 change: 1 addition & 0 deletions wdl-lint/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

* Added the `InconsistentNewlines` line rule ([#104](https://github.com/stjude-rust-labs/wdl/pull/104)).
* Add support for `#@ except` comments to disable lint rules ([#101](https://github.com/stjude-rust-labs/wdl/pull/101)).
* Added the `LineWidth` lint rule (#99).
* Added the `ImportWhitespace` and `ImportSort` lint rules (#98).
Expand Down
3 changes: 2 additions & 1 deletion wdl-lint/RULES.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ be out of sync with released packages.
| `ImportPlacement` | Clarity | Ensures that imports are placed between the version statement and any document items. |
| `ImportSort` | Clarity, Style | Ensures that imports are sorted lexicographically. |
| `ImportWhitespace` | Clarity, Style, Spacing | Ensures that there is no extraneous whitespace between or within imports. |
| `InputNotSorted` | Style | Ensures that input declarations are sorted |
| `InconsistentNewlines` | Clarity, Style | Ensures that newlines are used consistently within the file. |
| `InputNotSorted` | Style | Ensures that input declarations are sorted |
| `MatchingParameterMeta` | Completeness | Ensures that inputs have a matching entry in a `parameter_meta` section. |
| `MissingRuntime` | Completeness, Portability | Ensures that tasks have a runtime section. |
| `MissingMetas` | Completeness, Clarity | Ensures that tasks have both a meta and a parameter_meta section. |
Expand Down
1 change: 1 addition & 0 deletions wdl-lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ pub fn rules() -> Vec<Box<dyn Rule>> {
Box::new(rules::ImportSortRule),
Box::new(rules::InputNotSortedRule),
Box::new(rules::LineWidthRule::default()),
Box::new(rules::InconsistentNewlinesRule),
];

// Ensure all the rule ids are unique and pascal case
Expand Down
2 changes: 2 additions & 0 deletions wdl-lint/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ mod ending_newline;
mod import_placement;
mod import_sort;
mod import_whitespace;
mod inconsistent_newlines;
mod input_not_sorted;
mod line_width;
mod matching_parameter_meta;
Expand All @@ -25,6 +26,7 @@ pub use ending_newline::*;
pub use import_placement::*;
pub use import_sort::*;
pub use import_whitespace::*;
pub use inconsistent_newlines::*;
pub use input_not_sorted::*;
pub use line_width::*;
pub use matching_parameter_meta::*;
Expand Down
96 changes: 96 additions & 0 deletions wdl-lint/src/rules/inconsistent_newlines.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
//! A lint rule for ensuring that newlines are consistent.

use wdl_ast::AstToken;
use wdl_ast::Diagnostic;
use wdl_ast::Diagnostics;
use wdl_ast::Span;
use wdl_ast::VisitReason;
use wdl_ast::Visitor;
use wdl_ast::Whitespace;

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

/// The identifier for the inconsistent newlines rule.
const ID: &str = "InconsistentNewlines";

/// Creates an inconsistent newlines diagnostic.
fn inconsistent_newlines(span: Span) -> Diagnostic {
Diagnostic::note("inconsistent newlines detected")
.with_rule(ID)
.with_label("the first occurrence of a mismatched newline is here", span)
.with_fix("use either \"\\n\" or \"\\r\\n\" consistently in the file")
}

/// Detects imports that are not sorted lexicographically.
#[derive(Debug, Clone, Copy)]
pub struct InconsistentNewlinesRule;

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

fn description(&self) -> &'static str {
"Ensures that newline usage is consistent."
}

fn explanation(&self) -> &'static str {
"Files should not mix \\n and \\r\\n line breaks. Pick one and use it consistently in your \
project."
}

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

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

/// Implements the visitor for the import sort rule.
struct InconsistentNewlinesVisitor {
/// The number of carriage returns in the file.
carriage_return: u32,
/// The number of newlines in the file.
newline: u32,
/// Location of first inconsistent newline.
first_inconsistent: Option<Span>,
}

/// Implements the default inconsistent newlines visitor.
impl Default for InconsistentNewlinesVisitor {
fn default() -> Self {
Self {
carriage_return: 0,
newline: 0,
first_inconsistent: None,
}
}
}

impl Visitor for InconsistentNewlinesVisitor {
type State = Diagnostics;

fn document(&mut self, state: &mut Self::State, reason: VisitReason, _doc: &wdl_ast::Document) {
if reason == VisitReason::Exit && self.newline > 0 && self.carriage_return > 0 {
state.add(inconsistent_newlines(self.first_inconsistent.unwrap()));
}
}

fn whitespace(&mut self, _state: &mut Self::State, whitespace: &Whitespace) {
if let Some(pos) = whitespace.as_str().find("\r\n") {
self.carriage_return += 1;
if self.newline > 0 && self.first_inconsistent.is_none() {
self.first_inconsistent = Some(Span::new(whitespace.span().start() + pos, 2));
}
} else if let Some(pos) = whitespace.as_str().find('\n') {
self.newline += 1;
if self.carriage_return > 0 && self.first_inconsistent.is_none() {
self.first_inconsistent = Some(Span::new(whitespace.span().start() + pos, 1));
}
}
}
}
15 changes: 7 additions & 8 deletions wdl-lint/tests/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,13 @@ fn compare_result(path: &Path, result: &str, is_error: bool) -> Result<(), Strin

fn run_test(test: &Path, ntests: &AtomicUsize) -> Result<(), String> {
let path = test.join("source.wdl");
let source = std::fs::read_to_string(&path)
.map_err(|e| {
format!(
"failed to read source file `{path}`: {e}",
path = path.display()
)
})?
.replace("\r\n", "\n");
let source = std::fs::read_to_string(&path).map_err(|e| {
format!(
"failed to read source file `{path}`: {e}",
path = path.display()
)
})?;

match Document::parse(&source).into_result() {
Ok(document) => {
let mut validator = Validator::default();
Expand Down
9 changes: 9 additions & 0 deletions wdl-lint/tests/lints/inconsistent-newlines/source.errors
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
note[InconsistentNewlines]: inconsistent newlines detected
┌─ tests/lints/inconsistent-newlines/source.wdl:7:1
7 │ ╭
8 │ │ workflow foo {}
│ ╰^ the first occurrence of a mismatched newline is here
= fix: use either "/n" or "/r/n" consistently in the file

10 changes: 10 additions & 0 deletions wdl-lint/tests/lints/inconsistent-newlines/source.wdl
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#@ except: EndingNewline,PreambleWhitespace,MissingMetas,MissingOutput
## This is a test of the `InconsistentNewlines` lint
## Note that due an inexact path separator replacement in the tests,
## error messages in the baseline will show `/<escape>` instead of `\<escape>`.

version 1.1

workflow foo {}


Loading