Merge pull request #22 from sasjs/issue-16

feat: new rules noNestedMacros & hasMacroParentheses

.sasjslint

@@ -7,5 +7,7 @@
     "lowerCaseFileNames": true,
     "noTabIndentation": true,
     "indentationMultiple": 2,
-    "hasMacroNameInMend": false
+    "hasMacroNameInMend": false,
+    "noNestedMacros": true,
+    "hasMacroParentheses": true
 }

src/rules/hasMacroNameInMend.spec.ts

@@ -48,7 +48,7 @@ describe('hasMacroNameInMend', () => {
         message: 'mismatch macro name in %mend statement',
         lineNumber: 4,
         startColumnNumber: 9,
-        endColumnNumber: 25,
+        endColumnNumber: 24,
         severity: Severity.Warning
       }
     ])
@@ -219,7 +219,7 @@ describe('hasMacroNameInMend', () => {
           message: 'mismatch macro name in %mend statement',
           lineNumber: 6,
           startColumnNumber: 14,
-          endColumnNumber: 30,
+          endColumnNumber: 29,
           severity: Severity.Warning
         }
       ])

src/rules/hasMacroNameInMend.ts

@@ -2,6 +2,9 @@ import { Diagnostic } from '../types/Diagnostic'
 import { FileLintRule } from '../types/LintRule'
 import { LintRuleType } from '../types/LintRuleType'
 import { Severity } from '../types/Severity'
+import { trimComments } from '../utils/trimComments'
+import { getLineNumber } from '../utils/getLineNumber'
+import { getColNumber } from '../utils/getColNumber'
 
 const name = 'hasMacroNameInMend'
 const description = 'The %mend statement should contain the macro name'
@@ -22,8 +25,8 @@ const test = (value: string) => {
 
     if (trimmedStatement.startsWith('%macro ')) {
       const macroName = trimmedStatement
-        .split(' ')
-        .filter((s: string) => !!s)[1]
+        .slice(7, trimmedStatement.length)
+        .trim()
         .split('(')[0]
       stack.push(macroName)
     } else if (trimmedStatement.startsWith('%mend')) {
@@ -46,7 +49,7 @@ const test = (value: string) => {
           lineNumber: getLineNumber(statements, index + 1),
           startColumnNumber: getColNumber(statement, macroName),
           endColumnNumber:
-            getColNumber(statement, macroName) + macroName.length,
+            getColNumber(statement, macroName) + macroName.length - 1,
           severity: Severity.Warning
         })
       }
@@ -64,36 +67,6 @@ const test = (value: string) => {
   return diagnostics
 }
 
-const trimComments = (
-  statement: string,
-  commentStarted: boolean = false
-): { statement: string; commentStarted: boolean } => {
-  let trimmed = statement.trim()
-
-  if (commentStarted || trimmed.startsWith('/*')) {
-    const parts = trimmed.split('*/')
-    if (parts.length > 1) {
-      return {
-        statement: (parts.pop() as string).trim(),
-        commentStarted: false
-      }
-    } else {
-      return { statement: '', commentStarted: true }
-    }
-  }
-  return { statement: trimmed, commentStarted: false }
-}
-
-const getLineNumber = (statements: string[], index: number): number => {
-  const combinedCode = statements.slice(0, index).join(';')
-  const lines = (combinedCode.match(/\n/g) || []).length + 1
-  return lines
-}
-
-const getColNumber = (statement: string, text: string): number => {
-  return (statement.split('\n').pop() as string).indexOf(text) + 1
-}
-
 /**
  * Lint rule that checks for the presence of macro name in %mend statement.
  */

src/rules/hasMacroParentheses.spec.ts

@@ -0,0 +1,128 @@
+import { Severity } from '../types/Severity'
+import { hasMacroParentheses } from './hasMacroParentheses'
+
+describe('hasMacroParentheses', () => {
+  it('should return an empty array when macro defined correctly', () => {
+    const content = `
+  %macro somemacro();
+    %put &sysmacroname;
+  %mend somemacro;`
+
+    expect(hasMacroParentheses.test(content)).toEqual([])
+  })
+
+  it('should return an array with a single diagnostics when macro defined without parentheses', () => {
+    const content = `
+  %macro somemacro;
+    %put &sysmacroname;
+  %mend somemacro;`
+
+    expect(hasMacroParentheses.test(content)).toEqual([
+      {
+        message: 'Macro definition missing parentheses',
+        lineNumber: 2,
+        startColumnNumber: 10,
+        endColumnNumber: 18,
+        severity: Severity.Warning
+      }
+    ])
+  })
+
+  it('should return an array with a single diagnostics when macro defined without name', () => {
+    const content = `
+  %macro ();
+    %put &sysmacroname;
+  %mend;`
+
+    expect(hasMacroParentheses.test(content)).toEqual([
+      {
+        message: 'Macro definition missing name',
+        lineNumber: 2,
+        startColumnNumber: 3,
+        endColumnNumber: 12,
+        severity: Severity.Warning
+      }
+    ])
+  })
+
+  it('should return an array with a single diagnostics when macro defined without name and parentheses', () => {
+    const content = `
+  %macro ;
+    %put &sysmacroname;
+  %mend;`
+
+    expect(hasMacroParentheses.test(content)).toEqual([
+      {
+        message: 'Macro definition missing name',
+        lineNumber: 2,
+        startColumnNumber: 3,
+        endColumnNumber: 10,
+        severity: Severity.Warning
+      }
+    ])
+  })
+
+  it('should return an empty array when the file is undefined', () => {
+    const content = undefined
+
+    expect(hasMacroParentheses.test((content as unknown) as string)).toEqual([])
+  })
+
+  describe('with extra spaces and comments', () => {
+    it('should return an empty array when %mend has correct macro name', () => {
+      const content = `
+      /* 1st comment */
+     %macro  somemacro();
+
+      %put &sysmacroname;
+
+     /* 2nd 
+     comment */
+      /* 3rd comment */ %mend  somemacro ;`
+
+      expect(hasMacroParentheses.test(content)).toEqual([])
+    })
+
+    it('should return an array with a single diagnostic when macro defined without parentheses having code in comments', () => {
+      const content = `/**
+      @file examplemacro.sas
+      @brief an example of a macro to be used in a service
+      @details  This macro is great. Yadda yadda yadda.  Usage:
+
+        * code formatting applies when indented by 4 spaces; code formatting applies when indented by 4 spaces; code formatting applies when indented by 4 spaces; code formatting applies when indented by 4 spaces; code formatting applies when indented by 4 spaces;
+
+        some code
+        %macro examplemacro123();      
+        
+        %examplemacro()
+
+      <h4> SAS Macros </h4>
+      @li doesnothing.sas
+
+      @author Allan Bowe
+    **/
+
+    %macro examplemacro;
+
+    proc sql;
+    create table areas
+      as select area
+      
+    from sashelp.springs;
+
+    %doesnothing();
+
+    %mend;`
+
+      expect(hasMacroParentheses.test(content)).toEqual([
+        {
+          message: 'Macro definition missing parentheses',
+          lineNumber: 19,
+          startColumnNumber: 12,
+          endColumnNumber: 23,
+          severity: Severity.Warning
+        }
+      ])
+    })
+  })
+})

src/rules/hasMacroParentheses.ts

@@ -0,0 +1,77 @@
+import { Diagnostic } from '../types/Diagnostic'
+import { FileLintRule } from '../types/LintRule'
+import { LintRuleType } from '../types/LintRuleType'
+import { Severity } from '../types/Severity'
+import { trimComments } from '../utils/trimComments'
+import { getLineNumber } from '../utils/getLineNumber'
+import { getColNumber } from '../utils/getColNumber'
+
+const name = 'hasMacroParentheses'
+const description = 'Macros are always defined with parentheses'
+const message = 'Macro definition missing parentheses'
+const test = (value: string) => {
+  const diagnostics: Diagnostic[] = []
+
+  const statements: string[] = value ? value.split(';') : []
+
+  let trimmedStatement = '',
+    commentStarted = false
+  statements.forEach((statement, index) => {
+    ;({ statement: trimmedStatement, commentStarted } = trimComments(
+      statement,
+      commentStarted
+    ))
+
+    if (trimmedStatement.startsWith('%macro')) {
+      const macroNameDefinition = trimmedStatement
+        .slice(7, trimmedStatement.length)
+        .trim()
+
+      const macroNameDefinitionParts = macroNameDefinition.split('(')
+      const macroName = macroNameDefinitionParts[0]
+
+      if (!macroName)
+        diagnostics.push({
+          message: 'Macro definition missing name',
+          lineNumber: getLineNumber(statements, index + 1),
+          startColumnNumber: getColNumber(statement, '%macro'),
+          endColumnNumber: statement.length,
+          severity: Severity.Warning
+        })
+      else if (macroNameDefinitionParts.length === 1)
+        diagnostics.push({
+          message,
+          lineNumber: getLineNumber(statements, index + 1),
+          startColumnNumber: getColNumber(statement, macroNameDefinition),
+          endColumnNumber:
+            getColNumber(statement, macroNameDefinition) +
+            macroNameDefinition.length -
+            1,
+          severity: Severity.Warning
+        })
+      else if (macroName !== macroName.trim())
+        diagnostics.push({
+          message: 'Macro definition cannot have space',
+          lineNumber: getLineNumber(statements, index + 1),
+          startColumnNumber: getColNumber(statement, macroNameDefinition),
+          endColumnNumber:
+            getColNumber(statement, macroNameDefinition) +
+            macroNameDefinition.length -
+            1,
+          severity: Severity.Warning
+        })
+    }
+  })
+  return diagnostics
+}
+
+/**
+ * Lint rule that checks for the presence of macro name in %mend statement.
+ */
+export const hasMacroParentheses: FileLintRule = {
+  type: LintRuleType.File,
+  name,
+  description,
+  message,
+  test
+}

src/rules/noNestedMacros.spec.ts

@@ -0,0 +1,78 @@
+import { Severity } from '../types/Severity'
+import { noNestedMacros } from './noNestedMacros'
+
+describe('noNestedMacros', () => {
+  it('should return an empty array when no nested macro', () => {
+    const content = `
+  %macro somemacro();
+    %put &sysmacroname;
+  %mend somemacro;`
+
+    expect(noNestedMacros.test(content)).toEqual([])
+  })
+
+  it('should return an array with a single diagnostics when nested macro defined', () => {
+    const content = `
+    %macro outer();
+      /* any amount of arbitrary code */
+      %macro inner();
+        %put inner;
+      %mend;
+      %inner()
+      %put outer;
+    %mend;
+
+    %outer()`
+
+    expect(noNestedMacros.test(content)).toEqual([
+      {
+        message: "Macro definition present inside another macro 'outer'",
+        lineNumber: 4,
+        startColumnNumber: 7,
+        endColumnNumber: 20,
+        severity: Severity.Warning
+      }
+    ])
+  })
+
+  it('should return an array with a single diagnostics when nested macro defined 2 levels', () => {
+    const content = `
+    %macro outer();
+      /* any amount of arbitrary code */
+      %macro inner();
+        %put inner;
+
+                %macro inner2();
+                  %put inner2;
+                %mend;
+      %mend;
+      %inner()
+      %put outer;
+    %mend;
+
+    %outer()`
+
+    expect(noNestedMacros.test(content)).toEqual([
+      {
+        message: "Macro definition present inside another macro 'outer'",
+        lineNumber: 4,
+        startColumnNumber: 7,
+        endColumnNumber: 20,
+        severity: Severity.Warning
+      },
+      {
+        message: "Macro definition present inside another macro 'inner'",
+        lineNumber: 7,
+        startColumnNumber: 17,
+        endColumnNumber: 31,
+        severity: Severity.Warning
+      }
+    ])
+  })
+
+  it('should return an empty array when the file is undefined', () => {
+    const content = undefined
+
+    expect(noNestedMacros.test((content as unknown) as string)).toEqual([])
+  })
+})

src/rules/noNestedMacros.ts

@@ -0,0 +1,59 @@
+import { Diagnostic } from '../types/Diagnostic'
+import { FileLintRule } from '../types/LintRule'
+import { LintRuleType } from '../types/LintRuleType'
+import { Severity } from '../types/Severity'
+import { trimComments } from '../utils/trimComments'
+import { getLineNumber } from '../utils/getLineNumber'
+import { getColNumber } from '../utils/getColNumber'
+
+const name = 'noNestedMacros'
+const description = 'Defining nested macro is not good practice'
+const message = 'Macro definition present inside another macro'
+const test = (value: string) => {
+  const diagnostics: Diagnostic[] = []
+
+  const statements: string[] = value ? value.split(';') : []
+
+  const stack: string[] = []
+  let trimmedStatement = '',
+    commentStarted = false
+  statements.forEach((statement, index) => {
+    ;({ statement: trimmedStatement, commentStarted } = trimComments(
+      statement,
+      commentStarted
+    ))
+
+    if (trimmedStatement.startsWith('%macro ')) {
+      const macroName = trimmedStatement
+        .slice(7, trimmedStatement.length)
+        .trim()
+        .split('(')[0]
+      if (stack.length) {
+        const parentMacro = stack.slice(-1).pop()
+        diagnostics.push({
+          message: `${message} '${parentMacro}'`,
+          lineNumber: getLineNumber(statements, index + 1),
+          startColumnNumber: getColNumber(statement, '%macro'),
+          endColumnNumber:
+            getColNumber(statement, '%macro') + trimmedStatement.length - 1,
+          severity: Severity.Warning
+        })
+      }
+      stack.push(macroName)
+    } else if (trimmedStatement.startsWith('%mend')) {
+      stack.pop()
+    }
+  })
+  return diagnostics
+}
+
+/**
+ * Lint rule that checks for the presence of macro name in %mend statement.
+ */
+export const noNestedMacros: FileLintRule = {
+  type: LintRuleType.File,
+  name,
+  description,
+  message,
+  test
+}

src/types/LintConfig.spec.ts

@@ -58,6 +58,42 @@ describe('LintConfig', () => {
     expect(config.fileLintRules.length).toEqual(0)
   })
 
+  it('should create an instance with the noNestedMacros flag set', () => {
+    const config = new LintConfig({ noNestedMacros: true })
+
+    expect(config).toBeTruthy()
+    expect(config.lineLintRules.length).toEqual(0)
+    expect(config.fileLintRules.length).toEqual(1)
+    expect(config.fileLintRules[0].name).toEqual('noNestedMacros')
+    expect(config.fileLintRules[0].type).toEqual(LintRuleType.File)
+  })
+
+  it('should create an instance with the noNestedMacros flag off', () => {
+    const config = new LintConfig({ noNestedMacros: false })
+
+    expect(config).toBeTruthy()
+    expect(config.lineLintRules.length).toEqual(0)
+    expect(config.fileLintRules.length).toEqual(0)
+  })
+
+  it('should create an instance with the hasMacroParentheses flag set', () => {
+    const config = new LintConfig({ hasMacroParentheses: true })
+
+    expect(config).toBeTruthy()
+    expect(config.lineLintRules.length).toEqual(0)
+    expect(config.fileLintRules.length).toEqual(1)
+    expect(config.fileLintRules[0].name).toEqual('hasMacroParentheses')
+    expect(config.fileLintRules[0].type).toEqual(LintRuleType.File)
+  })
+
+  it('should create an instance with the hasMacroParentheses flag off', () => {
+    const config = new LintConfig({ hasMacroParentheses: false })
+
+    expect(config).toBeTruthy()
+    expect(config.lineLintRules.length).toEqual(0)
+    expect(config.fileLintRules.length).toEqual(0)
+  })
+
   it('should create an instance with the indentation multiple set', () => {
     const config = new LintConfig({ indentationMultiple: 5 })
 
@@ -82,7 +118,9 @@ describe('LintConfig', () => {
       maxLineLength: 80,
       noTabIndentation: true,
       indentationMultiple: 2,
-      hasMacroNameInMend: true
+      hasMacroNameInMend: true,
+      noNestedMacros: true,
+      hasMacroParentheses: true
     })
 
     expect(config).toBeTruthy()
@@ -98,11 +136,15 @@ describe('LintConfig', () => {
     expect(config.lineLintRules[4].name).toEqual('indentationMultiple')
     expect(config.lineLintRules[4].type).toEqual(LintRuleType.Line)
 
-    expect(config.fileLintRules.length).toEqual(2)
+    expect(config.fileLintRules.length).toEqual(4)
     expect(config.fileLintRules[0].name).toEqual('hasDoxygenHeader')
     expect(config.fileLintRules[0].type).toEqual(LintRuleType.File)
     expect(config.fileLintRules[1].name).toEqual('hasMacroNameInMend')
     expect(config.fileLintRules[1].type).toEqual(LintRuleType.File)
+    expect(config.fileLintRules[2].name).toEqual('noNestedMacros')
+    expect(config.fileLintRules[2].type).toEqual(LintRuleType.File)
+    expect(config.fileLintRules[3].name).toEqual('hasMacroParentheses')
+    expect(config.fileLintRules[3].type).toEqual(LintRuleType.File)
 
     expect(config.pathLintRules.length).toEqual(2)
     expect(config.pathLintRules[0].name).toEqual('noSpacesInFileNames')

src/types/LintConfig.ts

@@ -7,6 +7,8 @@ import { noSpacesInFileNames } from '../rules/noSpacesInFileNames'
 import { noTabIndentation } from '../rules/noTabIndentation'
 import { noTrailingSpaces } from '../rules/noTrailingSpaces'
 import { hasMacroNameInMend } from '../rules/hasMacroNameInMend'
+import { noNestedMacros } from '../rules/noNestedMacros'
+import { hasMacroParentheses } from '../rules/hasMacroParentheses'
 import { FileLintRule, LineLintRule, PathLintRule } from './LintRule'
 
 /**
@@ -61,5 +63,13 @@ export class LintConfig {
     if (json?.hasMacroNameInMend) {
       this.fileLintRules.push(hasMacroNameInMend)
     }
+
+    if (json?.noNestedMacros) {
+      this.fileLintRules.push(noNestedMacros)
+    }
+
+    if (json?.hasMacroParentheses) {
+      this.fileLintRules.push(hasMacroParentheses)
+    }
   }
 }

src/utils/getColNumber.ts

@@ -0,0 +1,3 @@
+export const getColNumber = (statement: string, text: string): number => {
+  return (statement.split('\n').pop() as string).indexOf(text) + 1
+}

src/utils/getLineNumber.ts

@@ -0,0 +1,5 @@
+export const getLineNumber = (statements: string[], index: number): number => {
+  const combinedCode = statements.slice(0, index).join(';')
+  const lines = (combinedCode.match(/\n/g) || []).length + 1
+  return lines
+}

src/utils/getLintConfig.spec.ts

@@ -17,7 +17,7 @@ describe('getLintConfig', () => {
     const config = await getLintConfig()
 
     expect(config).toBeInstanceOf(LintConfig)
-    expect(config.fileLintRules.length).toEqual(1)
+    expect(config.fileLintRules.length).toEqual(3)
     expect(config.lineLintRules.length).toEqual(5)
     expect(config.pathLintRules.length).toEqual(2)
   })

src/utils/getLintConfig.ts

@@ -15,7 +15,9 @@ export const DefaultLintConfiguration = {
   maxLineLength: 80,
   noTabIndentation: true,
   indentationMultiple: 2,
-  hasMacroNameInMend: false
+  hasMacroNameInMend: false,
+  noNestedMacros: true,
+  hasMacroParentheses: true
 }
 
 /**

src/utils/trimComments.ts

@@ -0,0 +1,19 @@
+export const trimComments = (
+  statement: string,
+  commentStarted: boolean = false
+): { statement: string; commentStarted: boolean } => {
+  let trimmed = statement.trim()
+
+  if (commentStarted || trimmed.startsWith('/*')) {
+    const parts = trimmed.split('*/')
+    if (parts.length > 1) {
+      return {
+        statement: (parts.pop() as string).trim(),
+        commentStarted: false
+      }
+    } else {
+      return { statement: '', commentStarted: true }
+    }
+  }
+  return { statement: trimmed, commentStarted: false }
+}