Break on conditional expression (#23)

 * Add LL3 rule for conditional expression breaking
* Cover more test cases
* Add ll3 to features.md
* Add to example_lint_cfg

Author: dtarassenko

example_files/example_lint_cfg.json

@@ -21,5 +21,6 @@
     "indent_continuation_line": {},
     "func_call_break_on_open_paren": {},
     "method_output_break": {},
+    "conditional_expression_break_before_op": {},
     "annotate_lints": true
 }

src/analysis/parsing/expression.rs

@@ -27,7 +27,7 @@ use crate::lint::{DMLStyleError,
                                     CurrentRules},
                                     AuxParams};
 use crate::lint::rules::indentation::IndentParenExprArgs;
-use crate::lint::rules::linebreaking::FuncCallBreakOnOpenParenArgs;
+use crate::lint::rules::linebreaking::{ConditionalExpressionBreakBeforeOperatorArgs, FuncCallBreakOnOpenParenArgs};
 
 #[derive(Debug, Clone, PartialEq)]
 pub struct UnaryExpressionContent {
@@ -159,6 +159,8 @@ impl TreeElement for TertiaryExpressionContent {
     }
     fn evaluate_rules(&self, acc: &mut Vec<DMLStyleError>, rules: &CurrentRules, _aux: AuxParams) {
         rules.sp_ternary.check(SpTernaryArgs::from_tertiary_expression_content(self), acc);
+        rules.conditional_expression_break_before_op
+            .check(ConditionalExpressionBreakBeforeOperatorArgs::from_tertiary_expression(self), acc);
     }
 }
 

src/lint/features.md

@@ -47,6 +47,7 @@ Below are listed the currently supported rules for linting:
 
 ## Line Length
 - **LL1**, `long_lines`: Lines should be kept shorter than 80 characters. This limit can be set to a custom value
+- **LL3**, `conditional_expression_break`: Break conditional expressions before the ?, or both before the ? and before the :.
 - **LL5**, `method_output_break`: Break long method declarations with output parameters before the arrow.
 ```
     method inquiry_status(uint64 physical_address)

src/lint/mod.rs

@@ -5,7 +5,7 @@ use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use lazy_static::lazy_static;
 use log::{debug, error, trace};
-use rules::linebreaking::{FuncCallBreakOnOpenParenOptions, MethodOutputBreakOptions};
+use rules::linebreaking::{FuncCallBreakOnOpenParenOptions, MethodOutputBreakOptions, ConditionalExpressionBreakBeforeOperatorOptions};
 use serde::{Deserialize, Serialize};
 use regex::Regex;
 use rules::{instantiate_rules, CurrentRules, RuleType};
@@ -117,6 +117,8 @@ pub struct LintCfg {
     #[serde(default)]
     pub func_call_break_on_open_paren: Option<FuncCallBreakOnOpenParenOptions>,
     #[serde(default)]
+    pub conditional_expression_break_before_op: Option<ConditionalExpressionBreakBeforeOperatorOptions>,
+    #[serde(default)]
     pub method_output_break: Option<MethodOutputBreakOptions>,
     #[serde(default = "get_true")]
     pub annotate_lints: bool,
@@ -166,12 +168,12 @@ impl Default for LintCfg {
             indent_continuation_line: Some(IndentContinuationLineOptions{indentation_spaces: INDENTATION_LEVEL_DEFAULT}),
             func_call_break_on_open_paren: Some(FuncCallBreakOnOpenParenOptions{indentation_spaces: INDENTATION_LEVEL_DEFAULT}),
             method_output_break: Some(MethodOutputBreakOptions{}),
+            conditional_expression_break_before_op: Some(ConditionalExpressionBreakBeforeOperatorOptions{}),
             annotate_lints: true,
         }
     }
 }
 
-
 #[derive(Debug, Clone)]
 pub struct DMLStyleError {
     pub error: LocalDMLError,

src/lint/rules/indentation.rs

@@ -731,7 +731,12 @@ impl IndentContinuationLineArgs {
     pub fn filter_out_last_semi_ranges(expression_tokens: &mut TreeElementTokenIterator) {
         // This function filters out the last semicolon in a list of tokens
         // as it is not part of the continuation line check.
-        expression_tokens.pop_if(|token| token.kind == TokenKind::SemiColon);
+        // expression_tokens.pop_if(|token| token.kind == TokenKind::SemiColon);
+        if let Some(last_token) = expression_tokens.last() {
+            if last_token.kind == TokenKind::SemiColon {
+                expression_tokens.pop();
+            }
+        }
     }
 
     pub fn from_statement_content(node: &StatementContent, depth: u32) -> Option<IndentContinuationLineArgs> {

src/lint/rules/linebreaking.rs

@@ -2,8 +2,9 @@ use serde::{Deserialize, Serialize};
 use crate::analysis::parsing::lexer::TokenKind;
 use crate::analysis::parsing::parser::Token;
 use crate::analysis::parsing::structure::MethodContent;
-use crate::analysis::parsing::tree::{TreeElement, TreeElementTokenIterator, ZeroRange};
-use crate::analysis::parsing::expression::{CastContent, FunctionCallContent, ParenExpressionContent};
+use crate::analysis::parsing::tree::{ZeroRange, TreeElementTokenIterator, TreeElement};
+use crate::analysis::parsing::expression::{CastContent, FunctionCallContent,
+                                           ParenExpressionContent, TertiaryExpressionContent};
 use crate::lint::{DMLStyleError, RuleType};
 use super::indentation::IndentParenExprArgs;
 use super::Rule;
@@ -214,3 +215,74 @@ impl FuncCallBreakOnOpenParenRule {
         }
     }
 }
+
+pub struct ConditionalExpressionBreakBeforeOperatorRule {
+    pub enabled: bool,
+}
+
+pub struct ConditionalExpressionBreakBeforeOperatorArgs {
+    pub left: ZeroRange,
+    pub left_operation: ZeroRange,
+    pub middle: ZeroRange,
+    pub right_operation: ZeroRange,
+    pub right: ZeroRange
+}
+
+
+impl ConditionalExpressionBreakBeforeOperatorArgs {
+    pub fn from_tertiary_expression(node: &TertiaryExpressionContent) 
+        -> Option<ConditionalExpressionBreakBeforeOperatorArgs> {
+        Some(ConditionalExpressionBreakBeforeOperatorArgs {
+            left: node.left.range(),
+            left_operation: node.left_operation.range(),
+            middle: node.middle.range(),
+            right_operation: node.right_operation.range(),
+            right: node.right.range(),
+        })
+    }
+}
+
+impl ConditionalExpressionBreakBeforeOperatorRule {
+    pub fn from_options(options: &Option<ConditionalExpressionBreakBeforeOperatorOptions>) -> ConditionalExpressionBreakBeforeOperatorRule {
+        match options {
+            Some(_options) => ConditionalExpressionBreakBeforeOperatorRule {
+                enabled: true,
+            },
+            None => ConditionalExpressionBreakBeforeOperatorRule {
+                enabled: false,
+            }
+        }
+    }
+
+    pub fn check(&self, args: Option<ConditionalExpressionBreakBeforeOperatorArgs>, acc: &mut Vec<DMLStyleError>) {
+        if !self.enabled { return; }
+        let Some(args) = args else { return; };
+        let has_break_before_question_operator = args.left.row_end.0 != args.left_operation.row_start.0;
+        let has_break_after_question_operator = args.left_operation.row_end.0 != args.middle.row_start.0;
+        let has_break_before_colon_operator = args.middle.row_end.0 != args.right_operation.row_start.0;
+        let has_break_after_colon_operator = args.right_operation.row_end.0 != args.right.row_start.0;
+        if has_break_after_question_operator {
+            acc.push(self.create_err(args.left_operation));
+        }
+        if has_break_after_colon_operator || (has_break_before_colon_operator && !has_break_before_question_operator ){
+            acc.push(self.create_err(args.right_operation));
+        }
+    }
+}
+
+
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
+pub struct ConditionalExpressionBreakBeforeOperatorOptions{
+}
+
+impl Rule for ConditionalExpressionBreakBeforeOperatorRule {
+    fn name() -> &'static str {
+        "COND_EXPRESSION_BREAK_BEFORE_OPERATOR"
+    }
+    fn description() -> &'static str {
+        "Break conditional expressions before the ?, or both before the ? and before the :."
+    }
+    fn get_rule_type() -> RuleType {
+        RuleType::LL3
+    }
+}

src/lint/rules/mod.rs

@@ -25,7 +25,8 @@ use indentation::{LongLinesRule,
     IndentEmptyLoopRule,
     IndentContinuationLineRule};
 use linebreaking::{FuncCallBreakOnOpenParenRule,
-    MethodOutputBreakRule};
+    MethodOutputBreakRule,
+    ConditionalExpressionBreakBeforeOperatorRule};
 use crate::lint::{LintCfg, DMLStyleError};
 use crate::analysis::{LocalDMLError, parsing::tree::ZeroRange};
 
@@ -51,6 +52,7 @@ pub struct CurrentRules {
     pub indent_continuation_line: IndentContinuationLineRule,
     pub func_call_break_on_open_paren: FuncCallBreakOnOpenParenRule,
     pub method_output_break: MethodOutputBreakRule,
+    pub conditional_expression_break_before_op: ConditionalExpressionBreakBeforeOperatorRule,
 }
 
 pub fn  instantiate_rules(cfg: &LintCfg) -> CurrentRules {
@@ -76,6 +78,7 @@ pub fn  instantiate_rules(cfg: &LintCfg) -> CurrentRules {
         indent_continuation_line: IndentContinuationLineRule::from_options(&cfg.indent_continuation_line),
         func_call_break_on_open_paren: FuncCallBreakOnOpenParenRule::from_options(&cfg.func_call_break_on_open_paren),
         method_output_break: MethodOutputBreakRule { enabled: cfg.method_output_break.is_some() },
+        conditional_expression_break_before_op: ConditionalExpressionBreakBeforeOperatorRule::from_options(&cfg.conditional_expression_break_before_op),
     }
 }
 
@@ -110,6 +113,7 @@ pub enum RuleType {
     NspUnary,
     NspTrailing,
     LL1,
+    LL3,
     LL5,
     LL6,
     IN2,

src/lint/rules/tests/line_length_breaking/conditional_expression_break.rs

@@ -0,0 +1,82 @@
+use crate::lint::rules::tests::common::{set_up, assert_snippet};
+use crate::lint::rules::RuleType;
+
+
+static CORRECT_BREAK_BEFORE_QUESTION_MARK: &str = "
+method bootprep_type_to_string(uint8 prep_type) -> (const char*) {
+    return
+        prep_type == PREP_GENERAL
+        ? \"PrepGeneral\" : \"PrepEarly\";
+}";
+#[test]
+fn condexpr_correct_break_before_question_mark() {
+    let rules = set_up();
+    let expected_errors = vec![];
+    assert_snippet(CORRECT_BREAK_BEFORE_QUESTION_MARK, expected_errors, &rules);
+}
+
+static CORRECT_BREAK_BEFORE_COLON_AND_QUESTION_MARK: &str = "
+method bootprep_type_to_string(uint8 prep_type) -> (const char*) {
+    return
+        prep_type == PREP_GENERAL
+        ? \"PrepGeneral\"
+        : \"PrepEarly\";
+}";
+#[test]
+fn condexpr_correct_break_after_colon_and_question_mark() {
+    let rules = set_up();
+    let expected_errors = vec![];
+    assert_snippet(CORRECT_BREAK_BEFORE_COLON_AND_QUESTION_MARK, expected_errors, &rules);
+}
+
+static BREAK_ONLY_BEFORE_COLON: &str = "
+method harvest_resource() {
+    harvest_resource(my->gas < GAS_THRESHOLD ? Rsrc_Gas
+                     : (my->minerals < MINERAL_THRESHOLD
+                        ? Rsrc_Minerals : nearest_resource()));
+}";
+#[test]
+fn condexpr_only_before_colon() {
+    let rules = set_up();
+    let expected_errors = define_expected_errors!(
+        RuleType::LL3,
+        (3, 3, 21, 22),
+    );
+    assert_snippet(BREAK_ONLY_BEFORE_COLON, expected_errors, &rules);
+}
+
+static CORRECT_NESTED: &str = "
+method bootprep_type_to_string(uint8 prep_type) -> (const char*) {
+    return
+        prep_type == PREP_GENERAL
+        ? \"PrepGeneral\"
+        : prep_type == PREP_EARLY
+        ? \"PrepEarly\" : \"UnknownBootPrepType\";
+}";
+#[test]
+fn condexpr_correct_nested() {
+    let rules = set_up();
+    let expected_errors = vec![];
+    assert_snippet(CORRECT_NESTED, expected_errors, &rules);
+}
+
+static BREAK_AFTER_OPERATORS_NESTED: &str = "
+method bootprep_type_to_string(uint8 prep_type) -> (const char*) {
+    return
+        prep_type == PREP_GENERAL ?
+        \"PrepGeneral\" :
+        prep_type == PREP_EARLY ? \"PrepEarly\" :
+        \"UnknownBootPrepType\";
+}";
+#[test]
+fn condexpr_broken_after_operators_nested() {
+    let rules = set_up();
+    let expected_errors = define_expected_errors!(
+        RuleType::LL3,
+        (3, 3, 34, 35),
+        (4, 4, 22, 23),
+        (5, 5, 46, 47),
+    );
+    assert_snippet(BREAK_AFTER_OPERATORS_NESTED, expected_errors, &rules);
+}
+

src/lint/rules/tests/line_length_breaking/mod.rs

@@ -1,2 +1,3 @@
 mod func_call_break_open_paren;
-mod method_output_break;
+mod method_output_break;
+mod conditional_expression_break;